[New platform] Support configuring request specific basePath #35951

mshustov · 2019-05-02T12:38:43Z

Summary

With this PR http service in NP provides ability:

to read / write request specific base path in any downstream plugin/core service
re-write request Url in onRequest interceptor

Both features are required to replace current Spaces functionality in legacy platform.
#35429 (comment)

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.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] 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
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

It's applied to identify incoming requests across New and Legacy platforms. We can rely on the value if want to attach additional information to incoming requests without mutating them.

elasticmachine · 2019-05-02T17:04:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-02T17:11:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-05-03T06:32:43Z

src/core/server/http/http_server.ts

+  }
+
+  // should work only for KibanaRequest as soon as spaces migrate to NP
+  private setBasePathFor(request: KibanaRequest | Request, basePath: string) {


We store basePath in WeakMap, where request.raw.req: IncomingMessage is used as a key. Hapi re-uses request.raw.req when we proxy request to LP, so it still can be used as a key to retrieve basePath from NP api

Can I ask why we opt for a WeakMap instead of a Map for this cache?

If we use ordinary Map it will lead to memory leaks, because Map keeps a reference to incoming request and it cannot be removed by GC. Using WeakMap frees us from the obligation to control the allocated memory manually.

mshustov · 2019-05-03T06:36:12Z

src/core/server/http/integration_tests/http_service.test.ts

-        __dirname,
-        './__fixtures__/plugins/dummy_on_request'
-      );
+      const plugin = path.resolve(__dirname, './__fixtures__/plugins/dummy_on_request');


TODO for myself: create an issue to cleanup tests. after #35297 we can use setup/ start contracts in tests and don't need to declare a plugin for this

created issue #36256

mshustov · 2019-05-03T06:39:41Z

retest

elasticmachine · 2019-05-03T07:33:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-05-03T11:49:39Z

src/core/server/http/router/request.ts

  }

  public getFilteredHeaders(headersToKeep: string[]) {
    return filterHeaders(this.headers, headersToKeep);
  }
+
+  // eslint-disable-next-line @typescript-eslint/camelcase
+  public unstable_getIncomingMessage() {


not sure we want to expose it on a long distance, so I added prefix unstable_

elasticmachine · 2019-05-03T11:54:24Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-05-03T11:54:25Z

Pinging @elastic/kibana-security

eliperelman

Have some syntax notes and question.

src/core/server/http/http_server.test.ts

eliperelman · 2019-05-03T14:53:28Z

src/core/server/http/http_server.test.ts

+  registerRouter(router);
+
+  await server.start(config);
+  await supertest(innerServer.listener)


Can this be re-written to take advantage of the async function and removing the then?

const response = await supertest(innerServer.listener).get('/').expect(200); expect(response.body).toEqual({ key: path });

yes, but in this case, the test suit is inconsistent from other tests in this file.

src/core/server/http/integration_tests/http_service.test.ts

eliperelman · 2019-05-03T15:02:34Z

src/core/server/http/http_server.ts

+  }
+
+  // should work only for KibanaRequest as soon as spaces migrate to NP
+  private setBasePathFor(request: KibanaRequest | Request, basePath: string) {


Can I ask why we opt for a WeakMap instead of a Map for this cache?

elasticmachine · 2019-05-05T12:23:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-06T10:03:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-05-06T17:42:13Z

@eliperelman addressed

eliperelman

LGTM with TODO addressed.

@eli

…#35951) * expose IncomingMessage from KibanaRequest It's applied to identify incoming requests across New and Legacy platforms. We can rely on the value if want to attach additional information to incoming requests without mutating them. * support attaching basePath information to incoming requests * Support Url changing for incoming requests * add tests * use NP API in the legacy platform * relax KibanaRequest typings * check basePath cannot be set twice * address @eli comments * generate docs

@eli

…#36257) * expose IncomingMessage from KibanaRequest It's applied to identify incoming requests across New and Legacy platforms. We can rely on the value if want to attach additional information to incoming requests without mutating them. * support attaching basePath information to incoming requests * Support Url changing for incoming requests * add tests * use NP API in the legacy platform * relax KibanaRequest typings * check basePath cannot be set twice * address @eli comments * generate docs

mshustov added 6 commits May 2, 2019 14:35

expose IncomingMessage from KibanaRequest

096a2e6

It's applied to identify incoming requests across New and Legacy platforms. We can rely on the value if want to attach additional information to incoming requests without mutating them.

support attaching basePath information to incoming requests

2bb07f2

Support Url changing for incoming requests

9d14cd5

add tests

89c7c46

use NP API in the legacy platform

b079cf0

relax KibanaRequest typings

5f26769

mshustov mentioned this pull request May 2, 2019

Spaces - New Platform Migration, Step 1 #35429

Merged

11 tasks

mshustov added 2 commits May 2, 2019 17:10

check basePath cannot be set twice

89898ad

Merge branch 'master' into expose-base-path-getters

d7bbc91

mshustov commented May 3, 2019

View reviewed changes

elastic deleted a comment from elasticmachine May 3, 2019

mshustov changed the title ~~Expose base path getters~~ [New platform] Support configuring request specific basePath May 3, 2019

mshustov commented May 3, 2019

View reviewed changes

mshustov added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Security/Spaces Platform Security - Spaces feature v7.2.0 labels May 3, 2019

mshustov marked this pull request as ready for review May 3, 2019 12:06

mshustov requested a review from a team as a code owner May 3, 2019 12:06

mshustov added the release_note:skip Skip the PR/issue when compiling release notes label May 3, 2019

eliperelman reviewed May 3, 2019

View reviewed changes

address @eli comments

5e07bc2

mshustov requested a review from eliperelman May 5, 2019 11:31

mshustov added 2 commits May 6, 2019 10:37

Merge branch 'master' into expose-base-path-getters

6184922

generate docs

42195af

eliperelman approved these changes May 7, 2019

View reviewed changes

mshustov merged commit 452f529 into elastic:master May 8, 2019

mshustov deleted the expose-base-path-getters branch May 8, 2019 07:28

mshustov mentioned this pull request May 8, 2019

[7.x] [New platform] Support configuring request specific basePath (#35951) #36257

Merged

rudolf added this to Done in kibana-core [DEPRECATED] May 30, 2019

legrego mentioned this pull request Jun 14, 2019

[New platform] Spaces plugin integration #39011

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New platform] Support configuring request specific basePath #35951

[New platform] Support configuring request specific basePath #35951

mshustov commented May 2, 2019 •

edited

elasticmachine commented May 2, 2019

elasticmachine commented May 2, 2019

mshustov May 3, 2019

eliperelman May 3, 2019

mshustov May 5, 2019

mshustov May 3, 2019

mshustov May 8, 2019

mshustov commented May 3, 2019

elasticmachine commented May 3, 2019

mshustov May 3, 2019

elasticmachine commented May 3, 2019

elasticmachine commented May 3, 2019

eliperelman left a comment

eliperelman May 3, 2019

mshustov May 5, 2019

eliperelman May 3, 2019

elasticmachine commented May 5, 2019

elasticmachine commented May 6, 2019

mshustov commented May 6, 2019

eliperelman left a comment •

edited

[New platform] Support configuring request specific basePath #35951

[New platform] Support configuring request specific basePath #35951

Conversation

mshustov commented May 2, 2019 • edited

Summary

Checklist

For maintainers

elasticmachine commented May 2, 2019

💚 Build Succeeded

elasticmachine commented May 2, 2019

💔 Build Failed

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

mshustov commented May 3, 2019

elasticmachine commented May 3, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented May 3, 2019

elasticmachine commented May 3, 2019

eliperelman 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

elasticmachine commented May 5, 2019

💚 Build Succeeded

elasticmachine commented May 6, 2019

💚 Build Succeeded

mshustov commented May 6, 2019

eliperelman left a comment • edited

Choose a reason for hiding this comment

mshustov commented May 2, 2019 •

edited

eliperelman left a comment •

edited