Spaces - New Platform Migration, Step 1 #35429

legrego · 2019-04-22T20:22:39Z

Summary

The first phase of this Spaces plugin migration to the new platform.
This is only concerned with part of the server side implementation. A client-side PR will follow.

The main goal of this PR is to migrate the onRequest interceptor in order to perform the equivalent of request.setBasePath() in the new platform. This will allow requests outside of the default space to be handled by new platform routes, and is a prerequisite to moving authentication to the new platform.

This PR does not reflect the final state of the spaces plugin. There are design shortcomings that will be addressed in subsequent PRs before the next minor release is cut, as the NP services are evolved and the security plugin is migrated alongside the spaces plugin.

Tasks

Create "New Platform" plugin
Create interfaces to abstract usage of Hapi server/request
Use the NP Elasticsearch service instead of the legacy ES plugin
Need NP equivalent of request.setBasePath() and request.getBasePath() in src/core - replace usages in Spaces plugin wherever possible
SpacesService.scopedClient() -- currently needs the full request object for subsequent callCluster invocations.
Migrate onRequest request interceptor
Testing

Not in scope

The following tasks need to be done, but are out of scope for this PR

Resolve or enable circular dependency between Spaces and Security
Migrate onPostAuth request interceptors to use NP, once available
Consolidate plugin services into SpacesService instead of exposing two functions on the server object.
Code re-org

elasticmachine · 2019-04-22T20:54:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-24T18:49:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-24T19:24:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-24T21:42:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

x-pack/plugins/spaces/server/lib/create_default_space.ts

x-pack/plugins/spaces/index.ts

legrego · 2019-04-29T14:55:00Z

@restrry thanks for agreeing to be my consultant!
Here is what I have so far for the Spaces migration.
I think my most immediate need/question is how we want to migrate request.setBasePath and request.getBasePath to the New Platform. They were introduced for Spaces, and allow callers to transparently get a space-aware base path to Kibana.

legrego · 2019-04-29T15:38:01Z

Actually, a more complex need is the ability to rewrite the incoming request before it is processed by the rest of the handler chain. We currently rely on the HAPI api and request shape to enable this today.

We re-write requests to drop the space identifier from the URL:

Browser: GET /s/marketing/app/kibana
Spaces interceptor:

strip off /s/marketing, store in request.setBasePath()
craft new url, omitting /s/marketing from the request.
send the updated request through the handler chain for subsequent processing by Kibana.

mshustov · 2019-04-29T12:59:30Z

x-pack/plugins/spaces/index.ts

-import { SavedObjectsService } from 'src/legacy/server/saved_objects';
+// @ts-ignore
+import KbnServer, { Server, KibanaConfig } from 'src/legacy/server/kbn_server';
+import { HttpServerInfo } from 'src/core/server/http';


use relative paths when import NP modules, please.
TODO for myself: enhance linting rule to restrict import from NP internals when import declared as an absolute path.

let's revert to absolute path import until the problem is resolved #35429 (comment)
also we shouldn't import from module internals, but the root level:

import { HttpServerInfo } from 'src/core/server';

x-pack/plugins/spaces/index.ts

x-pack/plugins/spaces/server/lib/get_spaces_usage_collector.ts

x-pack/plugins/spaces/server/routes/api/public/index.ts

x-pack/plugins/spaces/index.ts

x-pack/plugins/spaces/kibana.json

x-pack/plugins/spaces/server/lib/create_default_space.ts

elasticmachine · 2019-06-17T13:27:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

x-pack/plugins/spaces/index.ts

x-pack/plugins/spaces/server/new_platform/plugin.ts

kobelb · 2019-06-18T00:25:54Z

x-pack/plugins/spaces/server/lib/create_default_space.ts


-  const { getSavedObjectsRepository, SavedObjectsClient } = server.savedObjects;
+  const client = await elasticsearch.dataClient$.pipe(first()).toPromise();


I stumbled on this because the defaultSpaceExists check short-circuits the creation of the default space, but I was under the impression that the consumer of this function (which is still in legacy land) needs something to await, so the outcome of this function should still be a promise. If that's not the case, then maybe that simplifies things, but that's where I was getting stuck.

Since this is an async function, even if we return immediately, it'll still be a promise and everything will behave "alright". The biggest drawback that we get from the current implementation being promise based is that we don't get cancellation. If we were to rewrite watchStatusAndLicenseToInitialize to work with observables, we'd be able to cancel it half-way through. Right now, because we break out of the RxJs world, we don't get this...

kobelb · 2019-06-18T00:27:30Z

x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts

    };
-  });
+  }, 30000);


These timeouts are pretty long... do you happen to know what's causing the first test to take so long to run?

Yeah they are... I believe it's the call to root.start() which ends up taking a decent amount of time, because it's actually starting a server. I'm not sure why subsequent runs are faster than the first though

x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts

…cation

elasticmachine · 2019-06-18T12:52:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-18T13:55:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb

LGTM on green CI.

elasticmachine · 2019-06-18T18:46:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

legrego · 2019-06-18T19:25:48Z

retest

elasticmachine · 2019-06-18T20:31:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-19T11:08:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-19T13:22:17Z

x-pack/plugins/spaces/server/lib/create_default_space.test.ts

+    config: mockServer.config(),
+    savedObjects: (mockServer.savedObjects as unknown) as SavedObjectsService,
+    elasticsearch: ({
+      dataClient$: Rx.of({


TODO for myself: core should provide dataClients with mocked ClusterClient interface

mshustov · 2019-06-19T13:33:16Z

x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts

+        basePath = newPath;
+      });
+      httpMock.registerOnPreAuth = jest.fn().mockImplementation(async handler => {
+        const preAuthRequest = {


JFYI there is a hapi request mock, which is not exported outside yet. do you think we should provide KibanaRequest mock and export it as well? https://github.com/restrry/kibana/blob/2f22c48dbaec368b3e43a4a30bb50dd369b8e463/src/core/server/http/http_server.mocks.ts#L55

mshustov · 2019-06-19T13:48:42Z

x-pack/plugins/spaces/server/new_platform/spaces_service/spaces_service.ts

+type RequestFacade = KibanaRequest | Legacy.Request;
+
+export interface SpacesServiceSetup {
+  scopedClient(request: RequestFacade): Promise<SpacesClient>;


a bit misleading. SpacesClient cannot work with KibanaRequest, thus we can scope only to Legacy.Request

* crude test updates * remove custom server typedef * allow spaces to aquire security plugin after init * split CoreSetup into CoreSetup and PluginsSetup * move interfaces to new plugin * init interceptors in legacy plugin * fix import * add placeholder kibana.json * use NP Elasticsearch service instead of legacy ES Plugin * cleanup imports * don't destructure the es client * introduce request facade * document reason for getSecurity * prefer relative imports from src/core * fix typo in filename: inteceptors --> interceptors * fix imports; remove stray ts-ignore * improve typings for spaces client * rename InterfaceExcept --> Omit * don't use legacy config in NP * additional comment * shim NP config service * fix merge from master * revert relative imports into src/core and src/legacy * shim capabilities modifier into new platform * removing placeholder kibana.json * fix prettier problem * temporary: patch NP 'setUrl' * migrate onRequest interceptor to NP, without tests * fix ts error * testing and deps cleanup for onRequestInterceptor * replace spaces's usages of request.getBasePath with http.getBasePathFor * add explicit timeouts for jest interceptor tests * attempt to fix imports * use NP logging instead of faked implementation * revert stray yarn.lock change * attempt to stablize and fix tests * update jest config to include src/core/server/mocks * fix plugin config typings * add service tests * fix merge * allow spaces service to also work with legacy requests * update interfaces to confirm to new internal/external API convention * re-enable some post auth interceptor tests * add explicit timeouts for tests * prefer modifyUrl instead of manual url modification * update logger shim to conform to PluginInitializerContext * remove spaces ConfigClass * don't weaken type declaration for scoped cluster client calls * remove legacy server from SpacesCoreSetup * remove spaces service cache * remove legacy server as an interceptor dependency * use modifyUrl on the raw request too * remove unused import * cleanup typings * replace onRequest interceptor with new onPreAuth interceptor * fix onPostAuth tests * temporarily copy modifyUrl into spaces plugin * fix mock export * fix merge from master * spaces scopedClient always uses updated ES client and config * improve typings for usage collector * rename isLegacyRequest -> isFakeRequest * use updated NP base path API * remove commented code * only expose scoped spaces client * use OptionalPlugin instead of getSecurity * update imports of Saved Objects Service to use new src/core/server location * update core docs

legrego added 2 commits April 22, 2019 15:51

crude test updates

6b53cfb

remove custom server typedef

0de048e

legrego added 6 commits April 24, 2019 13:21

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

af427e5

allow spaces to aquire security plugin after init

1a56ee6

split CoreSetup into CoreSetup and PluginsSetup

d89f320

move interfaces to new plugin

d5cae9f

init interceptors in legacy plugin

929cf7b

fix import

751e4a7

legrego added 2 commits April 24, 2019 14:49

add placeholder kibana.json

ae7850a

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

6c75e85

legrego added 2 commits April 24, 2019 15:37

use NP Elasticsearch service instead of legacy ES Plugin

1e4cd95

cleanup imports

de35798

This comment has been minimized.

Sign in to view

don't destructure the es client

a0bd378

legrego commented Apr 29, 2019

View reviewed changes

x-pack/plugins/spaces/server/lib/create_default_space.ts Show resolved Hide resolved

x-pack/plugins/spaces/index.ts Outdated Show resolved Hide resolved

legrego added 2 commits April 29, 2019 10:21

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

4e835a4

introduce request facade

5d27c18

This comment has been minimized.

Sign in to view

mshustov reviewed Apr 29, 2019

View reviewed changes

legrego added 5 commits April 29, 2019 11:59

document reason for getSecurity

661fe99

prefer relative imports from src/core

0ce0e47

fix typo in filename: inteceptors --> interceptors

d7ca4ba

fix imports; remove stray ts-ignore

600a33a

improve typings for spaces client

c5a8656

use updated NP base path API

4546013

kobelb reviewed Jun 18, 2019

View reviewed changes

legrego added 7 commits June 18, 2019 06:52

remove commented code

a1260b5

only expose scoped spaces client

a0d2d70

use OptionalPlugin instead of getSecurity

c245dca

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

cc85d11

update imports of Saved Objects Service to use new src/core/server lo…

d0e87a5

…cation

update core docs

a04b6ed

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

174e2c8

kobelb approved these changes Jun 18, 2019

View reviewed changes

legrego added 3 commits June 18, 2019 10:25

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

dae35da

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

a9ea28f

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

1e21cc8

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

2f22c48

mshustov approved these changes Jun 19, 2019

View reviewed changes

legrego merged commit 7e4e8fe into elastic:master Jun 19, 2019

legrego deleted the np/spaces-part-1 branch June 19, 2019 14:08

legrego restored the np/spaces-part-1 branch June 19, 2019 14:09

legrego mentioned this pull request Jun 19, 2019

[7.x] Spaces - New Platform Migration, Step 1 (#35429) #39270

Merged

legrego deleted the np/spaces-part-1 branch June 19, 2019 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces - New Platform Migration, Step 1 #35429

Spaces - New Platform Migration, Step 1 #35429

legrego commented Apr 22, 2019 •

edited

Loading

elasticmachine commented Apr 22, 2019

elasticmachine commented Apr 24, 2019

elasticmachine commented Apr 24, 2019

This comment has been minimized.

elasticmachine commented Apr 24, 2019

legrego commented Apr 29, 2019

This comment has been minimized.

legrego commented Apr 29, 2019

mshustov Apr 29, 2019

mshustov May 6, 2019

elasticmachine commented Jun 17, 2019

kobelb Jun 18, 2019

kobelb Jun 18, 2019

legrego Jun 18, 2019

elasticmachine commented Jun 18, 2019

elasticmachine commented Jun 18, 2019

kobelb left a comment

elasticmachine commented Jun 18, 2019

legrego commented Jun 18, 2019

elasticmachine commented Jun 18, 2019

elasticmachine commented Jun 19, 2019

mshustov Jun 19, 2019

mshustov Jun 19, 2019

mshustov Jun 19, 2019


		const { getSavedObjectsRepository, SavedObjectsClient } = server.savedObjects;
		const client = await elasticsearch.dataClient$.pipe(first()).toPromise();

Spaces - New Platform Migration, Step 1 #35429

Spaces - New Platform Migration, Step 1 #35429

Conversation

legrego commented Apr 22, 2019 • edited Loading

Summary

Tasks

Not in scope

elasticmachine commented Apr 22, 2019

💔 Build Failed

elasticmachine commented Apr 24, 2019

💔 Build Failed

elasticmachine commented Apr 24, 2019

💔 Build Failed

This comment has been minimized.

elasticmachine commented Apr 24, 2019

💚 Build Succeeded

legrego commented Apr 29, 2019

This comment has been minimized.

legrego commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 18, 2019

💔 Build Failed

elasticmachine commented Jun 18, 2019

💔 Build Failed

kobelb left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 18, 2019

💔 Build Failed

legrego commented Jun 18, 2019

elasticmachine commented Jun 18, 2019

💔 Build Failed

elasticmachine commented Jun 19, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented Apr 22, 2019 •

edited

Loading