[Session Management] Fix session index creation and update #208244

SiddharthMantri · 2025-01-24T17:24:11Z

Closes #208243

Summary

This PR introduced an unintended issue for users running 8.x with a 7.x archive. This has been fixed by correctly searching for existing index by both name and alias.

To test:

You can follow the same steps as the above PR: #204097

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

See some risk examples
...

elasticmachine · 2025-01-24T17:27:24Z

Pinging @elastic/kibana-security (Team:Security)

SiddharthMantri · 2025-01-27T07:45:59Z

@azasypkin Adding you as an optional extra reviewer here.

azasypkin · 2025-01-27T09:59:21Z

ACK: reviewing...

azasypkin · 2025-01-27T10:54:04Z

x-pack/platform/plugins/shared/security/server/session_management/session_index.ts

        index: this.aliasName,
      });
+      const indexNameExists = await this.options.elasticsearchClient.indices.exists({


issue: We shouldn’t check if the index exists if aliasNameExists is true, so let’s try to skip making the second call if possible.

Makes sense. Done in 0113bd6

azasypkin · 2025-01-27T11:23:41Z

x-pack/platform/plugins/shared/security/server/session_management/session_index.ts

  /**
   * Creates the session index if it doesn't already exist.
   */
  private async ensureSessionIndexExists() {
    // Check if required index exists.
    let indexExists = false;
    try {
-      indexExists = await this.options.elasticsearchClient.indices.exists({
+      const aliasNameExists = await this.options.elasticsearchClient.indices.exists({


question: I see docs claim that the method supports array of strings: https://www.elastic.co/docs/api/doc/elasticsearch/v8/operation/operation-indices-exists - would it be possible to for us to check both in a single call (not sure how that's supposed to work under the hood though... if it returns true only if all indices exist then it wouldn't work for our use case I guess)?

@azasypkin The exists API returns true only if all the targets are present. If any is missing, it returns false.

azasypkin · 2025-01-27T11:42:26Z

x-pack/platform/plugins/shared/security/server/session_management/session_index.ts

+        index: this.indexName,
+      });
+
+      indexExists = aliasNameExists || indexNameExists;


question: Would you mind clarifying in the code comment which case we’re handling here and why?

question: Do we need this change in main or it'd be sufficient to have this change only in 8.x considering that it's not possible to use 7.x data with 9.0 directly without going through UA? Is there any benefits in having this change in main that I'm missing?

I wasn't too comfortable with having this difference between 8.x and 9.0 as it felt "significant" enough. What do you recommend?

Code comment added in 0113bd6

azasypkin · 2025-01-28T17:40:30Z

x-pack/platform/plugins/shared/security/server/session_management/session_index.ts

  /**
   * Creates the session index if it doesn't already exist.
   */
  private async ensureSessionIndexExists() {
    // Check if required index exists.
    let indexExists = false;
+    let indexNameExists = false;


nit: How about this?

// It is possible for users to migrate from older versions of Kibana where the session index was created without // an alias (pre-8.4). In this case, we need to check if the index exists under the alias name, or the index name. // If the index exists under the alias name, we can assume that the alias is already attached. let indexExists = false; try { indexExists = (await this.options.elasticsearchClient.indices.exists({ index: this.aliasName })) || (await this.options.elasticsearchClient.indices.exists({ index: this.indexName })); } catch (err) { this.options.logger.error(`Failed to check if session index exists: ${err.message}`); throw err; }

Done in 689dd48

azasypkin · 2025-01-28T18:00:06Z

x-pack/platform/plugins/shared/security/server/session_management/session_index.test.ts

@@ -96,7 +96,10 @@ describe('Session index', () => {



note: Would you mind adding a couple of tests (or adjusting the existing ones) to validate that we call exists only once if the alias exists and is called with the alias name, and twice if the alias doesn’t exist and is called with both the alias and index names?

Added in 689dd48

azasypkin

LGTM with a few minor nits, thanks! Tested locally by loading Kibana 8.3 and then using the same data folder for 8.x with and without the changes in this PR - everything works as expected with the fix!

x-pack/platform/plugins/shared/security/server/session_management/session_index.test.ts

SiddharthMantri · 2025-01-29T13:32:59Z

@elasticmachine merge upstream

kibanamachine · 2025-01-29T15:28:47Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13034686386

elasticmachine · 2025-01-29T15:29:11Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 6ff6419

Failed CI Steps

Jest Integration Tests #1

Test Failures

[job] [logs] Jest Integration Tests #1 / unrecognized task types should be no workload aggregator errors when there are removed task types

Metrics [docs]

✅ unchanged

History

💛 Build #271371 was flaky 689dd48
💚 Build #270873 succeeded 701e312
💔 Build #270826 failed 0113bd6
💚 Build #270151 succeeded 684d61a

…08244) Closes elastic#208243 ## Summary [This PR](elastic#204097) introduced an unintended issue for users running 8.x with a 7.x archive. This has been fixed by correctly searching for existing index by both name and alias. ### To test: You can follow the same steps as the above PR: elastic#204097 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... (cherry picked from commit 449ac98)

kibanamachine · 2025-01-29T15:34:19Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…8244) (#208780) # Backport This will backport the following commits from `main` to `8.x`: - [[Session Management] Fix session index creation and update (#208244)](#208244)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Sid <[email protected]>

Sessions] Fix session index creation and update

684d61a

SiddharthMantri requested a review from a team as a code owner January 24, 2025 17:24

SiddharthMantri changed the title ~~Sessions] Fix session index creation and update~~ [Session Management] Fix session index creation and update Jan 24, 2025

SiddharthMantri added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes Feature:Security/Session Management Platform Security - Session Management labels Jan 24, 2025

SiddharthMantri added backport backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) and removed backport labels Jan 24, 2025

SiddharthMantri requested a review from azasypkin January 27, 2025 07:45

azasypkin reviewed Jan 27, 2025

View reviewed changes

update check for alias and index

0113bd6

SiddharthMantri requested a review from azasypkin January 28, 2025 08:25

fix logic for checking index exists

701e312

azasypkin reviewed Jan 28, 2025

View reviewed changes

Update tests, fix checking logic

689dd48

SiddharthMantri requested a review from azasypkin January 29, 2025 08:10

azasypkin approved these changes Jan 29, 2025

View reviewed changes

SiddharthMantri added 2 commits January 29, 2025 12:32

Address PR comments

8dc3fdd

updated mock impl

5fffe18

SiddharthMantri enabled auto-merge (squash) January 29, 2025 13:32

Merge branch 'main' into fix-session-index-creation

6ff6419

SiddharthMantri merged commit 449ac98 into elastic:main Jan 29, 2025
8 checks passed

kibanamachine added the v9.0.0 label Jan 29, 2025

kibanamachine mentioned this pull request Jan 29, 2025

[8.x] [Session Management] Fix session index creation and update (#208244) #208780

Merged

kibanamachine added the v8.18.0 label Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Session Management] Fix session index creation and update #208244

[Session Management] Fix session index creation and update #208244

SiddharthMantri commented Jan 24, 2025 •

edited by kibanamachine

Loading

elasticmachine commented Jan 24, 2025

SiddharthMantri commented Jan 27, 2025

azasypkin commented Jan 27, 2025

azasypkin Jan 27, 2025

SiddharthMantri Jan 28, 2025

azasypkin Jan 27, 2025

SiddharthMantri Jan 28, 2025

azasypkin Jan 27, 2025

SiddharthMantri Jan 28, 2025

SiddharthMantri Jan 28, 2025

azasypkin Jan 28, 2025

SiddharthMantri Jan 29, 2025

azasypkin Jan 28, 2025

SiddharthMantri Jan 29, 2025

azasypkin left a comment

SiddharthMantri commented Jan 29, 2025

kibanamachine commented Jan 29, 2025

elasticmachine commented Jan 29, 2025

kibanamachine commented Jan 29, 2025

[Session Management] Fix session index creation and update #208244

[Session Management] Fix session index creation and update #208244

Conversation

SiddharthMantri commented Jan 24, 2025 • edited by kibanamachine Loading

Summary

To test:

Checklist

Identify risks

elasticmachine commented Jan 24, 2025

SiddharthMantri commented Jan 27, 2025

azasypkin commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

SiddharthMantri commented Jan 29, 2025

kibanamachine commented Jan 29, 2025

elasticmachine commented Jan 29, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

kibanamachine commented Jan 29, 2025

💚 All backports created successfully

Questions ?

SiddharthMantri commented Jan 24, 2025 •

edited by kibanamachine

Loading