[server/rewriteBasePath] Support rewriting basePath requests #16724

spalger · 2018-02-13T23:59:26Z

For some reason server.basePath expects the proxy sending the request to rewrite the request and strip the basePath. Unfortunately, users constantly encounter this expectation with surprise and many waste a good amount of time wondering why Kibana isn't responding properly.

The new server.rewriteBasePath setting enables request rewriting in the Kibana server so that Kibana will respond as expected when it receives requests starting with the server.baseRoute. For BWC server.rewriteBasePath is false by default, and setting server.basePath without setting server.rewriteBasePath will cause a deprecation warning to be logged.

In 7.0 server.rewriteBasePath will default to true and the deprecation warning will go away (a PR will follow this one to enable this behavior for 7.0).

Release Note: Adds support for rewriting the server.basePath within in the Kibana server so you no longer need to rewrite urls in your reverse proxy. To control this behavior in Kibana 6.3+ add server.rewriteBasePath: true|false to your Kibana config file. If you are using server.basePath and don't enable or disable the server.rewriteBasePath setting in your config file then a deprecation warning will be logged at startup because this setting will default to true in Kibana 7.0.

elasticmachine · 2018-02-14T00:50:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-14T02:05:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-02-15T08:42:44Z

config/kibana.yml

+# Specifies whether Kibana should rewrite requests that are prefixed with
+# `server.basePath` or require that they are rewritten by your reverse proxy.
+# This setting was effectively always `false` before Kibana 6.3 and will
+# default to `true` starting in Kibana 7.0


nit: full stop after Kibana 7.0?

azasypkin · 2018-02-15T08:43:05Z

docs/setup/settings.asciidoc

- the URLs generated by Kibana, your proxy is expected to remove the basePath value before forwarding requests
- to Kibana. This setting cannot end in a slash (`/`).
+`server.basePath:`:: Enables you to specify a path to mount Kibana at if you are running behind a proxy. Use the `server.rewriteBasePath` setting to tell Kibana if it should remove the basePath from requests it receives, and to prevent a deprecation warning at startup. This setting cannot end in a slash (`/`).
+`server.rewriteBasePath:`:: *Default: false* Specifies whether Kibana should rewrite requests that are prefixed with `server.basePath` or require that they are rewritten by your reverse proxy. This setting was effectively always `false` before Kibana 6.3 and will default to `true` starting in Kibana 7.0


nit: full stop after Kibana 7.0?

azasypkin · 2018-02-15T09:00:21Z

src/server/config/schema.js

@@ -53,6 +53,11 @@ export default () => Joi.object({
    autoListen: Joi.boolean().default(true),
    defaultRoute: Joi.string().default('/app/kibana').regex(/^\//, `start with a slash`),
    basePath: Joi.string().default('').allow('').regex(/(^$|^\/.*[^\/]$)/, `start with a slash, don't end with one`),
+    rewriteBasePath: Joi.boolean().when('basePath', {
+      is: Joi.valid(''),


nit: can it be just is: ''?

azasypkin · 2018-02-15T13:36:54Z

src/server/config/transform_deprecations.js

@@ -25,6 +25,18 @@ const savedObjectsIndexCheckTimeout = (settings, log) => {
  }
 };

+const rewriteBasePath = (settings, log) => {
+  if (_.has(settings, 'server.basePath') && !_.has(settings, 'server.rewriteBasePath')) {
+    log(


question: is there any magic step (apart from defining deprecated keys in config obviously) I should make to see this warning? I don't see any deprecation warnings defined in this file for some reason :/

Wow, well, looks like the server isn't logging deprecation messages... #16795. I think you'll just have to verify the code until @epixa gets to #16795

Haha, yeah code lgtm

azasypkin · 2018-02-15T14:17:56Z

src/server/http/__tests__/base_path_rewrite.js

@@ -0,0 +1,188 @@
+import { Server } from 'hapi';


nit: not sure what is our convention here, but shouldn't this test file have the same name as the file it tests?

azasypkin · 2018-02-15T14:20:31Z

src/server/http/__tests__/base_path_rewrite.js

+
+  describe('no base path', () => {
+    it('/bar => 404', async () => {
+      const server = createServer({ basePath: '', rewriteBasePath: false });


nit: maybe move const server = createServer({ basePath: '', rewriteBasePath: false }); to beforeEach (for this suite and suites below)?

azasypkin · 2018-02-15T14:25:41Z

src/server/http/__tests__/base_path_rewrite.js

+      expect(resp.payload).to.be('resp:/');
+    });
+
+    it('/bar/ => 404', async () => {


nit: wrong description

azasypkin · 2018-02-15T14:25:51Z

src/server/http/__tests__/base_path_rewrite.js

+      expect(resp.payload).to.be('resp:/');
+    });
+
+    it('/bar/foo => 404', async () => {


nit: wrong description

azasypkin

LGTM

…ver-base-route

jbudz · 2018-02-16T20:48:16Z

src/server/config/transform_deprecations.js

+    log(
+      'Setting server.basePath without server.rewriteBasePath is deprecated until Kibana 7.0, ' +
+      'when the default behavior of server.basePath will change and Kibana will start expecting ' +
+      'that all requests start with the server.basePath rather expecting you to rewrite the ' +


nit, rather than expecting

Also maybe a nit, I found it hard to follow this sentence. Would it be easier to read if it said,

You should set server.basePath along with server.rewriteBasePath. From Kibana 7.0 onward, server.rewriteBasePath defaults to true, even if not set; Kibana will start expecting that all requests start with the server.basePath rather than expecting you to rewrite the requests in your reverse proxy. Explicitly set server.rewriteBasePath to false to preserve the current behavior and silence this warning.

I would 👍 as it is, leaving it up to you, of course.

Thanks @archanid, I like your phasing in certain parts a lot more. I'll merge the two.

rhoboat · 2018-02-16T20:52:26Z

docs/setup/settings.asciidoc

- the URLs generated by Kibana, your proxy is expected to remove the basePath value before forwarding requests
- to Kibana. This setting cannot end in a slash (`/`).
+`server.basePath:`:: Enables you to specify a path to mount Kibana at if you are running behind a proxy. Use the `server.rewriteBasePath` setting to tell Kibana if it should remove the basePath from requests it receives, and to prevent a deprecation warning at startup. This setting cannot end in a slash (`/`).
+`server.rewriteBasePath:`:: *Default: false* Specifies whether Kibana should rewrite requests that are prefixed with `server.basePath` or require that they are rewritten by your reverse proxy. This setting was effectively always `false` before Kibana 6.3 and will default to `true` starting in Kibana 7.0.


rhoboat · 2018-02-16T20:52:59Z

src/cli/cluster/cluster_manager.js

@@ -22,12 +22,14 @@ export default class ClusterManager {
      this.basePathProxy = new BasePathProxy(this, settings);

      optimizerArgv.push(
-        `--server.basePath=${this.basePathProxy.basePath}`
+        `--server.basePath=${this.basePathProxy.basePath}`,
+        '--server.rewriteBasePath=true',


thanks for trailing commas

rhoboat · 2018-02-16T20:54:11Z

src/server/config/__tests__/schema.js

+        const { error } = validate({ server: { rewriteBasePath: 'foo' } });
+        expect(error).to.have.property('details');
+        expect(error.details[0]).to.have.property('path', 'server.rewriteBasePath');
+      });


should there be a test case rejecting everything but strings for basePath?

rhoboat · 2018-02-16T20:54:40Z

src/server/config/schema.js

+    rewriteBasePath: Joi.boolean().when('basePath', {
+      is: '',
+      then: Joi.default(false).valid(false),
+      otherwise: Joi.default(false)


nit: trailing comma?

elasticmachine · 2018-02-16T21:11:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rhoboat · 2018-02-16T21:19:41Z

Looks like you have two LGTMs, so I'm leaving mine off--I haven't run the code yet. But code LGTM!

…ver-base-route

…asePath

elasticmachine · 2018-02-19T18:51:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…#16724) * [server/rewriteBasePath] add option to enable basePath rewriting * [server/rewriteBasePath/docs] end sentences with periods * [server/rewriteBasePath] simplify Joi schema a smidge * [server/rewriteBasePath] rename test file to match source * [server/rewriteBasePath] initialize server in before/after hooks * [server/rewriteBasePath] rephrase deprecation warning * [server/config/schema] verify that non-strings are not accepted for basePath * [server/config/schema] toss a trailing comma in there

…#16814) * [server/rewriteBasePath] add option to enable basePath rewriting * [server/rewriteBasePath/docs] end sentences with periods * [server/rewriteBasePath] simplify Joi schema a smidge * [server/rewriteBasePath] rename test file to match source * [server/rewriteBasePath] initialize server in before/after hooks * [server/rewriteBasePath] rephrase deprecation warning * [server/config/schema] verify that non-strings are not accepted for basePath * [server/config/schema] toss a trailing comma in there

spalger · 2018-02-19T21:43:49Z

6.3/6.x: ae0358a

kibana/elastic#16724 added a basepath rewrite to the kibana server, and any route that doesn't go through hapi needs to deal with it explicitly

spalger added release_note:enhancement review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.3.0 labels Feb 13, 2018

spalger requested review from tylersmalley and azasypkin February 13, 2018 23:59

spalger force-pushed the implement/server-base-route branch from 6768f58 to 1f8ae3a Compare February 14, 2018 00:02

[server/rewriteBasePath] add option to enable basePath rewriting

3c07d08

spalger force-pushed the implement/server-base-route branch from 1f8ae3a to 3c07d08 Compare February 14, 2018 01:16

spalger changed the title ~~[server/baseRoute] Enhanced version of basePath, also rewrites requests~~ [server/rewriteBasePath] Support rewriting basePath requests Feb 14, 2018

azasypkin reviewed Feb 15, 2018

View reviewed changes

azasypkin approved these changes Feb 15, 2018

View reviewed changes

spalger added 5 commits February 16, 2018 12:40

Merge branch 'master' of github.com:elastic/kibana into implement/ser…

6afdcd5

…ver-base-route

[server/rewriteBasePath/docs] end sentences with periods

9053b5e

[server/rewriteBasePath] simplify Joi schema a smidge

0a9a64c

[server/rewriteBasePath] rename test file to match source

ace596d

[server/rewriteBasePath] initialize server in before/after hooks

49e642c

spalger requested review from jbudz and rhoboat and removed request for tylersmalley February 16, 2018 20:19

jbudz reviewed Feb 16, 2018

View reviewed changes

jbudz approved these changes Feb 16, 2018

View reviewed changes

rhoboat reviewed Feb 16, 2018

View reviewed changes

spalger added 4 commits February 19, 2018 10:56

Merge branch 'master' of github.com:elastic/kibana into implement/ser…

03c12ae

…ver-base-route

[server/rewriteBasePath] rephrase deprecation warning

d845c62

[server/config/schema] verify that non-strings are not accepted for b…

d168119

…asePath

[server/config/schema] toss a trailing comma in there

b93d1ed

spalger merged commit 1516e07 into elastic:master Feb 19, 2018

spalger mentioned this pull request Feb 19, 2018

[6.x] [server/rewriteBasePath] Support rewriting basePath requests (#16724) #16814

Merged

spalger deleted the implement/server-base-route branch February 19, 2018 21:43

w33ble added a commit to w33ble/kibana that referenced this pull request Sep 13, 2018

fix: handle basepath rewriting for socket connection

3a407cf

kibana/elastic#16724 added a basepath rewrite to the kibana server, and any route that doesn't go through hapi needs to deal with it explicitly

jbudz mentioned this pull request Jan 31, 2019

Add an option to change the root prefix for NodeJS (Phusion Passenger compatibility) #10587

Closed

mpereira mentioned this pull request Mar 26, 2019

DCOS-48854: Upgrade Elastic base-tech to 6.6.1. mesosphere/dcos-commons#2976

Merged

This was referenced Oct 9, 2021

Remove deprecated Core settings #103915

Closed

Update rewriteBasePath documentation #114562

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server/rewriteBasePath] Support rewriting basePath requests #16724

[server/rewriteBasePath] Support rewriting basePath requests #16724

spalger commented Feb 13, 2018 •

edited

Loading

elasticmachine commented Feb 14, 2018

elasticmachine commented Feb 14, 2018

azasypkin Feb 15, 2018

azasypkin Feb 15, 2018

azasypkin Feb 15, 2018

azasypkin Feb 15, 2018 •

edited

Loading

spalger Feb 16, 2018

azasypkin Feb 16, 2018

azasypkin Feb 15, 2018

azasypkin Feb 15, 2018

azasypkin Feb 15, 2018

azasypkin Feb 15, 2018

azasypkin left a comment

jbudz Feb 16, 2018

rhoboat Feb 16, 2018 •

edited

Loading

spalger Feb 19, 2018

rhoboat Feb 16, 2018

rhoboat Feb 16, 2018

rhoboat Feb 16, 2018

rhoboat Feb 16, 2018

elasticmachine commented Feb 16, 2018

rhoboat commented Feb 16, 2018

elasticmachine commented Feb 19, 2018

spalger commented Feb 19, 2018

[server/rewriteBasePath] Support rewriting basePath requests #16724

[server/rewriteBasePath] Support rewriting basePath requests #16724

Conversation

spalger commented Feb 13, 2018 • edited Loading

elasticmachine commented Feb 14, 2018

💚 Build Succeeded

elasticmachine commented Feb 14, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Feb 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

rhoboat Feb 16, 2018 • edited Loading

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

elasticmachine commented Feb 16, 2018

💚 Build Succeeded

rhoboat commented Feb 16, 2018

elasticmachine commented Feb 19, 2018

💚 Build Succeeded

spalger commented Feb 19, 2018

spalger commented Feb 13, 2018 •

edited

Loading

azasypkin Feb 15, 2018 •

edited

Loading

rhoboat Feb 16, 2018 •

edited

Loading