Stop relying on index template for session index #134900

legrego · 2022-06-22T12:14:26Z

Summary

This moves the index settings and mappings into the session index directly, rather than defining them on an index template. This also removes the index template if it already exists.

elasticmachine · 2022-06-22T17:28:10Z

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

legrego

Author's notes for reviewers

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

legrego · 2022-06-22T16:10:35Z

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

+  /**
+   * Creates the session index if it doesn't already exist.
+   */
+  private async ensureSessionIndexExists() {


note This was extracted from the initialize function above so that it can be reused in the create function.

legrego · 2022-06-22T17:32:06Z

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

@@ -213,17 +209,30 @@ export class SessionIndex {
        'Attempted to create a new session while session index is initializing.'
      );
      await this.indexInitialization;
+    } else {
+      await this.ensureSessionIndexExists();


I don't love the extra round-trip that this incurs, but I don't see a way around it given the design of ES's APIs.

kc13greiner

LGTM!

…-session-index-template

legrego · 2022-06-27T16:44:22Z

@azasypkin this is ready for the next round of reviews

azasypkin

Looks and works great! Thanks for adding tests to verify all aspects of a new functionality.

No need to change anything is this PR, just wondering if it'd make sense to (eventually) use index alias for the rest of the session operations as well (get, update, invalidate)?

azasypkin · 2022-06-28T07:33:11Z

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

+          'Attempted to create a new session, but session index or alias was missing.'
+        );
+        await this.ensureSessionIndexExists();
+        ({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, {


nit: statusCode isn't used here

Suggested change

({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, {

({ body } = await this.writeNewSessionDocument(sessionValue, {

Looks like you unintentionally returned unused statusCode back 🙂

Looks like you unintentionally returned unused statusCode back 🙂

This is actually required to avoid an eslint error, because both body and statusCode are part of the same let block above. If I remove statusCode here, it complains that statusCode isn't a const above. I could either fix this by reintroducing the unused statusCode reference here, or break up the destructured assignment above into multiple lines. Reintroducing statusCode here felt less bad to me, but I don't feel strongly about this if you would prefer that I change it around.

This is actually required to avoid an eslint error, because both body and statusCode are part of the same let block above. If I remove statusCode here, it complains that statusCode isn't a const above.

Ahh, I see now, thanks for clarifying! Please, keep it as is then.

azasypkin · 2022-06-28T07:37:29Z

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

+      // Prior to https://github.com/elastic/kibana/pull/134900, sessions would be written directly against the session index.
+      // Now, we write sessions against a new session index alias. This call ensures that the alias exists, and is attached to the index.
+      // This operation is safe to repeat, even if the alias already exists. This seems safer than retrieving the index details, and inspecting
+      // it to see if the alias already exists.


note: thank you for adding comment! I agree using putAlias feels safer.

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

legrego · 2022-06-28T11:59:34Z

@azasypkin,

No need to change anything is this PR, just wondering if it'd make sense to (eventually) use index alias for the rest of the session operations as well (get, update, invalidate)?

I was wondering the same thing. If we were writing this from scratch today, would we opt to do this? If so, then I'm happy to attempt these changes as part of this PR. If it turns out to be a lot of work, then I can open a followup issue for one of us to tackle in the future.

I can't think of a good reason not to do this, other than "unknown unknowns".

Co-authored-by: Aleh Zasypkin <[email protected]>

…-session-index-template

azasypkin · 2022-06-28T12:17:57Z

If we were writing this from scratch today, would we opt to do this?

I'd say yes.

If so, then I'm happy to attempt these changes as part of this PR. If it turns out to be a lot of work, then I can open a followup issue for one of us to tackle in the future.

Sounds good to me, thank you!

azasypkin · 2022-06-28T16:12:43Z

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

+        ignore404: true,
+      });
+
+      if (statusCode === 404) {


question: do we handle 404 for updates just to account for a case when the alias is missing, but the index exists? I mean if index doesn't exist then the session we're trying to update doesn't exist either...

Yes my intention here was to handle the case where we have an index without an alias

Got it, thanks. We usually retrieve session (via alias now) before we update it, but there is still a possibility for the race condition and handling 404 in update wouldn't hurt anyway 👍

azasypkin

Looks great! Would it make sense to also use alias in the cleanUp and getSessionValuesInBatches methods?

azasypkin · 2022-06-29T08:46:03Z

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

+          'Attempted to create a new session, but session index or alias was missing.'
+        );
+        await this.ensureSessionIndexExists();
+        ({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, {


Looks like you unintentionally returned unused statusCode back 🙂

azasypkin · 2022-06-29T08:46:22Z

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

+        ignore404: true,
+      });
+
+      if (statusCode === 404) {


Got it, thanks. We usually retrieve session (via alias now) before we update it, but there is still a possibility for the race condition and handling 404 in update wouldn't hurt anyway 👍

legrego · 2022-06-29T12:54:05Z

Looks great! Would it make sense to also use alias in the cleanUp and getSessionValuesInBatches methods?

AHHH How did I miss those? 😢

…-session-index-template

kibana-ci · 2022-06-29T19:12:08Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #54246 succeeded c4c0b4d
💚 Build #53849 succeeded 9040699
💔 Build #53822 failed 3e34e6a
💚 Build #53466 succeeded 33d26b6
💚 Build #52903 succeeded f10e428
💚 Build #52785 succeeded b890529

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

azasypkin

Remove session index template

c25e378

legrego added 2 commits June 22, 2022 11:33

Attempt to ensure session index exists before creating session document

10c0ed1

improved comments

667c565

legrego marked this pull request as ready for review June 22, 2022 17:28

legrego requested a review from a team as a code owner June 22, 2022 17:28

legrego added the v8.4.0 label Jun 22, 2022

legrego commented Jun 22, 2022

View reviewed changes

legrego requested review from azasypkin and kc13greiner June 22, 2022 17:46

kc13greiner reviewed Jun 22, 2022

View reviewed changes

legrego added 4 commits June 23, 2022 09:13

Attempt to use index alias + require_alias

b890529

more docs and testing

47f3393

Merge branch 'main' of github.com:elastic/kibana into security/remove…

f10e428

…-session-index-template

Merge branch 'main' into security/remove-session-index-template

33d26b6

azasypkin approved these changes Jun 28, 2022

View reviewed changes

legrego and others added 2 commits June 28, 2022 08:01

Apply suggestions from code review

3e34e6a

Co-authored-by: Aleh Zasypkin <[email protected]>

Merge branch 'main' of github.com:elastic/kibana into security/remove…

deb7d46

…-session-index-template

legrego added 3 commits June 28, 2022 08:27

update session via alias

0b37e65

get session via alias

a69acc6

invalidate session via alias

9040699

legrego requested a review from azasypkin June 28, 2022 14:05

azasypkin reviewed Jun 28, 2022

View reviewed changes

azasypkin approved these changes Jun 29, 2022

View reviewed changes

legrego added 2 commits June 29, 2022 09:22

use alias in cleanup and getSessionValuesInBatches

ed48c6a

Merge branch 'main' of github.com:elastic/kibana into security/remove…

c4c0b4d

…-session-index-template

legrego mentioned this pull request Jun 29, 2022

Kibana authentication troubleshooting guide #83914

Open

legrego requested a review from azasypkin June 29, 2022 16:08

use alias for index refresh

1411a68

azasypkin approved these changes Jun 30, 2022

View reviewed changes

legrego merged commit 9ba6aa9 into elastic:main Jun 30, 2022

legrego deleted the security/remove-session-index-template branch June 30, 2022 09:39

yakhinvadim pushed a commit to yakhinvadim/kibana that referenced this pull request Jul 5, 2022

Stop relying on index template for session index (elastic#134900)

c034896

rudolf mentioned this pull request Aug 21, 2023

Ignore user templates on Kibana system indices elastic/elasticsearch#98695

Closed

azasypkin mentioned this pull request Aug 25, 2023

Revert "Kibana system index does not allow user templates to affect it (#98696)" elastic/elasticsearch#98888

Merged

rudolf mentioned this pull request Aug 28, 2023

Block user templates from affecting kibana system indices #164949

Open

azasypkin mentioned this pull request Nov 1, 2023

Kibana should not auto create it's system indices if these don't exist #81790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop relying on index template for session index #134900

Stop relying on index template for session index #134900

legrego commented Jun 22, 2022

elasticmachine commented Jun 22, 2022

legrego left a comment

legrego Jun 22, 2022

legrego Jun 22, 2022

kc13greiner left a comment

legrego commented Jun 27, 2022

azasypkin left a comment

azasypkin Jun 28, 2022

azasypkin Jun 29, 2022

legrego Jun 29, 2022

azasypkin Jun 29, 2022

azasypkin Jun 28, 2022

legrego commented Jun 28, 2022

azasypkin commented Jun 28, 2022

azasypkin Jun 28, 2022

legrego Jun 28, 2022

azasypkin Jun 29, 2022

azasypkin left a comment

azasypkin Jun 29, 2022

azasypkin Jun 29, 2022

legrego commented Jun 29, 2022

kibana-ci commented Jun 29, 2022

azasypkin left a comment

	({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, {
	({ body } = await this.writeNewSessionDocument(sessionValue, {

Stop relying on index template for session index #134900

Stop relying on index template for session index #134900

Conversation

legrego commented Jun 22, 2022

Summary

elasticmachine commented Jun 22, 2022

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kc13greiner left a comment

Choose a reason for hiding this comment

legrego commented Jun 27, 2022

azasypkin left a 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

legrego commented Jun 28, 2022

azasypkin commented Jun 28, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented Jun 29, 2022

kibana-ci commented Jun 29, 2022

💚 Build Succeeded

Metrics [docs]

History

azasypkin left a comment

Choose a reason for hiding this comment