Spaces NP Migration - Moving server to the LegacyAPI model by legrego · Pull Request #45382 · elastic/kibana

legrego · 2019-09-11T13:58:26Z

Summary

Inspired by @azasypkin's work on the security plugin migration, this updates the spaces server code to use the LegacyAPI approach to better delineate what we have left to migrate. It also allowed for additional cleanup so that I can move the server code to a real NP plugin in a followup PR.

Notable changes:

The space selector screen is no longer rendered at the Kibana root, but rather at /spaces/space_selector. Having a dedicated route for this allowed the onPostAuth request interceptor to move to the NP, as it was previously rendering a hidden app inside of the interceptor. Now, the interceptor redirects to a legacy route (/spaces/space_selector) which is responsible for rendering the view.

…ector path

legrego · 2019-09-11T15:04:20Z

    async init(server: Server) {
      const kbnServer = (server as unknown) as KbnServer;
-      const initializerContext = ({
-        legacyConfig: server.config(),


legacyConfig moves to LegacyAPI.legacyConfig

legrego · 2019-09-11T15:05:00Z

-        usage: server.usage,
-        tutorial: {
-          addScopedTutorialContextFactory: server.addScopedTutorialContextFactory,
+      const core: CoreSetup = ({


Now that all legacy requirements have moved to LegacyAPI, we can remove SpacesCoreSetup in favor of the real CoreSetup provided by NP

legrego · 2019-09-11T15:06:25Z

      };

-      const { spacesService, log } = await plugin(initializerContext).setup(core, plugins);
+      const legacyRouter = server.route.bind(server);


The only component that couldn't move into LegacyAPI is the legacy router, because the route definitions are created before the legacy API is initialized. This is very short lived though, as my next PR will move the server code to a real NP plugin, which will use the NP router.

Btw, aren't you registering routes in setupLegacyComponents that is called from registerLegacyAPI?

elasticmachine · 2019-09-11T19:00:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-11T19:13:12Z

Pinging @elastic/kibana-security

azasypkin · 2019-09-12T11:01:25Z

ACK: will review today

azasypkin

Looks great! Just few minor nits/questions. Briefly tested locally and everything worked as expected, will test more thoroughly when draft turns into ready-to-review PR.

azasypkin · 2019-09-12T14:51:05Z

    response: LifecycleResponseFactory,
    toolkit: OnPreAuthToolkit
  ) {
+    const serverBasePath: string = getLegacyAPI().legacyConfig.get('server.basePath');


note: do we not use http.basePath.get here intentionally (it seems it can work here as expected)? And if so, what is our plan/replacement in NP?

I think it would technically work, as this interceptor hasn't rewritten the base path yet. It felt clearer to me that we were looking for the server's base path by reading this config directly, as opposed to relying on the implicit behavior of the execution order here.

as opposed to relying on the implicit behavior of the execution order here.

Agree and it works for me, I'm just curious what we'll do in NP since we don't have anything except for basePath service there afaik.

Yeah, good question.

@restrry what do you think? I know we have plans for exposing these values to the client, but do you know how we can get access to things like server.basePath from within server-side plugins?

For this specific property, we could make http.basePath.get take an optional request object, instead of a required one, but that won't solve the general problem.

The spaces plugin also needs access to the kibana.index property too, in order to collect its telemetry metrics. Is this property exposed via a core service somewhere?

Can't we just make serverBasePath property of BasePath service as public readonly to not confuse it with "request" base path, but at the same time still keep them together?

Having access to kibana.index is listed as a dependency for the Security plugin as well. If it stays in Kibana plugin, then we need just Kibana NP plugin that would expose that for the time being (like we did with Timelion NP plugin and timelion.ui.enabled config key).

Can't we just make serverBasePath property of BasePath service as public readonly to not confuse it with "request" base path, but at the same time still keep them together?

Yep, that's a good idea!

Having access to kibana.index is listed as a dependency for the Security plugin as well. If it stays in Kibana plugin, then we need just Kibana NP plugin that would expose that for the time being (like we did with Timelion NP plugin and timelion.ui.enabled config key).

Yeah that'd work too. So maybe the answer here is that plugins and services will just expose the properties as needed, rather than introducing a whitelisting system for the yml entries in general.

I've created #45991

azasypkin · 2019-09-12T15:18:31Z

@@ -11,10 +11,12 @@ import {
  HttpServiceSetup,


note: not related to this PR, but it seems these tests should be within a integration_tests folder (these are run with jest_integration.js and have a larger default timeout so that you don't need to increase it explicitly).

Ah I didn't know this -- thanks!

Build failed due to timeout when I did this and removed the explicit timeout increases :(
https://kibana-ci.elastic.co/job/elastic+kibana+pull-request/2706/

Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.

Hmm, maybe they weren't picked up by jest_integration for some reason? Are they run when you use node scripts/jest_integration.js locally?

hmm, apparently not:

elastic-mbp:x-pack larry$ node scripts/jest_integration.js ./legacy/plugins/spaces/server/lib/request_interceptors/ /Users/larry/repos/kibana/x-pack/scripts/jest_integration.js:19 throw new Error(` ^ Error: jest_integration tests have been disabled because of a flaky failure in the example integration test: https://github.com/elastic/kibana/issues/32795#issuecomment-471585274 when un-skipping these tests make sure to uncomment test/scripts/jenkins_xpack.sh lines 33-37 at Object.<anonymous> (/Users/larry/repos/kibana/x-pack/scripts/jest_integration.js:19:7) at Module._compile (internal/modules/cjs/loader.js:689:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10) at Module.load (internal/modules/cjs/loader.js:599:32) at tryModuleLoad (internal/modules/cjs/loader.js:538:12) at Function.Module._load (internal/modules/cjs/loader.js:530:3) at Function.Module.runMain (internal/modules/cjs/loader.js:742:12) at startup (internal/bootstrap/node.js:283:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)

Ugh, didn't know that 😭 Sorry for wasting your time on that

No worries! I'll just revert it

…timeout handling

…acy-api

elasticmachine · 2019-09-12T19:21:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rudolf

👍 For platform changes

legrego · 2019-09-13T16:30:25Z

@azasypkin ready for another review whenever you get a chance

azasypkin · 2019-09-16T07:02:12Z

ACK: will review today

legrego · 2019-09-16T13:05:03Z

@azasypkin you can wait on the review -- I thought through a couple of different scenarios that I want to verify before you spend more time on this.

azasypkin

@azasypkin you can wait on the review -- I thought through a couple of different scenarios that I want to verify before you spend more time on this.

Ha, actually code LGTM, just few nits. Let me know when I can start testing it locally then.

azasypkin · 2019-09-16T12:04:19Z

      };

-      const { spacesService, log } = await plugin(initializerContext).setup(core, plugins);
+      const legacyRouter = server.route.bind(server);


Btw, aren't you registering routes in setupLegacyComponents that is called from registerLegacyAPI?

…acy-api

legrego · 2019-09-16T14:16:50Z

      return {
        available,
-        enabled: spacesAvailableAndEnabled, // similar behavior as _xpack API in ES
+        enabled: available,


It's not technically necessary to have both available and enabled here, but I chose to keep it in so that telemetry consumers can have a consistent schema when searching for spaces-related statistics.

legrego · 2019-09-16T14:20:18Z

The changes in 48b0091 weren't strictly required as part of this PR, as this behavior has likely existed since the initial spaces NP shim, but this gives us the original implementation that we intended, and it'll help out when we go to implement #44678

elasticmachine · 2019-09-16T15:52:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-09-17T12:11:51Z

@azasypkin now this is ready for you :)

azasypkin

LGTM! Tested locally and haven't noticed anything suspicious. Just two notes:

We return 404 when default is used as a space ID in URL, but redirect to space selector when space isn't known. It probably works as intended since we do this in master, just got curious
When users delete the last non-default space we redirect them to space selector in this PR (with only Default space to choose from), but to the root (without space selector) in master. It's super edge case and doesn't feel like a big deal though.

legrego · 2019-09-18T12:16:29Z

We return 404 when default is used as a space ID in URL, but redirect to space selector when space isn't known. It probably works as intended since we do this in master, just got curious

That is curious! I'm glad I didn't make it any worse, but I'll see if I can track that down.

When users delete the last non-default space we redirect them to space selector in this PR (with only Default space to choose from), but to the root (without space selector) in master. It's super edge case and doesn't feel like a big deal though.

Good catch! I agree it's an edge case, but I'd still like to solve this. I think it'll be best to tackle this once #45991 merges, since at that point I'll be able to send users back to the root path again without introducing another legacy shim.

legrego · 2019-09-18T15:10:52Z

@elasticmachine update branch

elasticmachine · 2019-09-18T16:49:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…5382)

moving to the legacy api model

3dcd3ef

legrego added the Feature:NP Migration label Sep 11, 2019

legrego added 2 commits September 11, 2019 10:59

update onPostAuth error handling

f58b05d

cleaning up onRequest interceptor tests

115ea0c

This comment has been minimized.

Sign in to view

updating UI Capabilities tests to expect redirection to new space sel…

1b619a6

…ector path

legrego commented Sep 11, 2019

View reviewed changes

pass status code correctly

4962e51

This comment has been minimized.

Sign in to view

update snapshot...

2e4f6f4

This comment has been minimized.

Sign in to view

legrego added release_note:skip Skip the PR/issue when compiling release notes Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.5.0 v8.0.0 labels Sep 11, 2019

legrego requested a review from azasypkin September 11, 2019 19:14

azasypkin reviewed Sep 12, 2019

View reviewed changes

legrego added 9 commits September 12, 2019 12:38

remove unused interface

b4939e3

removing ts-ignore

b2ed48f

marking plugin fields readonly

940742d

removing unnecessary types

1099e3f

improving type safety of wrapError

6d26d9f

simplifying legacy config access

ea16257

moving interceptor tests into an integration_tests folder for better …

87d7ae7

…timeout handling

Merge branch 'master' of github.com:elastic/kibana into np/spaces-leg…

7ae92e2

…acy-api

remove hard-coded test credentials

3d8729b

legrego requested review from a team as code owners September 13, 2019 13:14

legrego requested a review from azasypkin September 13, 2019 13:52

rudolf approved these changes Sep 13, 2019

View reviewed changes

legrego mentioned this pull request Sep 13, 2019

Support space-specific default routes #44678

Merged

azasypkin reviewed Sep 16, 2019

View reviewed changes

legrego added 5 commits September 16, 2019 09:43

removing spaces enabled check from usage collector

4c30bfc

removing unnecessary types

81629bc

moving legacy router into LegacyAPI

0a8ba3f

improve handling when navigating to the root of a non-default space

48b0091

Merge branch 'master' of github.com:elastic/kibana into np/spaces-leg…

42c617f

…acy-api

legrego commented Sep 16, 2019

View reviewed changes

rudolf mentioned this pull request Sep 18, 2019

[New Platform] Expose serverBasePath #45991

Closed

azasypkin approved these changes Sep 18, 2019

View reviewed changes

Merge branch 'master' into np/spaces-legacy-api

5770f84

legrego merged commit 15e52f2 into elastic:master Sep 18, 2019

legrego deleted the np/spaces-legacy-api branch September 18, 2019 16:57

legrego mentioned this pull request Sep 18, 2019

[7.x] Spaces NP Migration - Moving server to the LegacyAPI model (#45382) #46045

Merged

legrego added a commit to legrego/kibana that referenced this pull request Sep 18, 2019

Spaces NP Migration - Moving server to the LegacyAPI model (elastic#4…

9d8f044

…5382)

legrego mentioned this pull request Sep 20, 2019

Spaces - respond with ES headers on space retrieval failure #46238

Merged

patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026

Spaces NP Migration - Moving server to the LegacyAPI model (elastic#4…

a7970db

…5382)

Conversation

legrego commented Sep 11, 2019

Summary

Notable changes:

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Sep 11, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 11, 2019

Uh oh!

azasypkin commented Sep 12, 2019

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Sep 12, 2019

💔 Build Failed

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

legrego commented Sep 13, 2019

Uh oh!

azasypkin commented Sep 16, 2019

Uh oh!

legrego commented Sep 16, 2019

Uh oh!

azasypkin Sep 13, 2019 •

edited

Loading

azasypkin Sep 13, 2019 •

edited

Loading