Add `server.publicBaseUrl` config #85075

joshdover · 2020-12-04T21:10:46Z

Summary

Fixes first part of #45815

This adds a new server.publicBaseUrl configuration option and exposes it to plugins via the core.basePath.publicBaseUrl on both the client and server sides.

A couple of points for discussion:

Currently, this configuration option is not required and Core does not attempt to provide a default to plugins. This is intentional so that plugins may determine the best solution when the user has not explicitly configured this.
This configuration is currently validated in a way that ensures that the configured URL matches the server.basePath if configured. Alternatively, we could only require that the user configures the protocol, host, and port and we could automatically append the basePath to this URL. I couldn't decide which error state would be less confusing to the user:
```
server.basePath: '/foo'
server.publicBaseUrl: 'http://myhost.com'
# Error!
```
```
server.basePath: '/foo'
server.publicBaseUrl: 'http://myhost.com/foo'
# Error!
```
We could also try to be smart here and only append the basePath if no pathname is already present, but I prefer to keep things simple here to eliminate any confusion that could arise.

To be done in follow up PRs:

Add a warning notification if the user is accessing Kibana via a host, protocol, or port that does not match the default of http://localhost:5601 Add "network.publish_host" style configuration in Kibana #45815 (comment)
Audit current usages of core.http.getServerInfo() and determine if publicBaseUrl is more appropriate in those usages. Add "network.publish_host" style configuration in Kibana #45815 (comment)

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-12-04T21:10:49Z

Pinging @elastic/kibana-core (Team:Core)

mikecote

I encountered an issue during my testing, see comment below.

In the PR description, I think it should be core.http.basePath.publicBaseUrl instead of core.basePath.publicBaseUrl?

config/kibana.yml

…ase-url

pgayvallet · 2020-12-07T18:51:21Z

This configuration is currently validated in a way that ensures that the configured URL matches the server.basePath if configured

I'm wondering if that's a technical limitation or an opinionated decision?

I could see scenarios where Kibana is using a basePath, but behind a reverse proxy, and where the public url does not have the said basePath?

joshdover · 2020-12-07T19:35:05Z

I'm wondering if that's a technical limitation or an opinionated decision?

I could see scenarios where Kibana is using a basePath, but behind a reverse proxy, and where the public url does not have the said basePath?

I added this validation explicitly, so we should be able to do whatever we'd like here. My goal here is to make getting this config right as easy as possible (or for it to be obvious when it is wrong).

The case you mentioned, I'm not too familiar with why someone would want to do that, but I do understand it is possible. Without any data to back up not supporting that, we should probably allow that deployment topology somehow. I see two options:

Remove the validation checks completely and allow publicBaseUrl to be configured with any path.
Add a config option to opt-out of this verification. Something like server.allowBasePathMismatch. The only reason for going with this option would be if we think/know that this deployment topology is atypical, and we want the typical topology to be a bit more failsafe.

Given that we plan to add a client-side warning if the user accesses Kibana with a different option than what is configured in server.publicBaseUrl, I lean towards just removing the validation.

mikecote

I finished my local testing for alerting's use case and it works for us 👍 thanks!

…ase-url

pgayvallet · 2020-12-08T12:34:59Z

The case you mentioned, I'm not too familiar with why someone would want to do that, but I do understand it is possible. Without any data to back up not supporting that, we should probably allow that deployment topology somehow.

I'm honestly not sure this is even a real need, I was mostly asking. I'm even wondering if everything would work fine on the client-side with such configuration, as we are doing a lot of checks based on the serverBasePath

joshdover · 2020-12-08T17:43:33Z

I'm honestly not sure this is even a real need, I was mostly asking. I'm even wondering if everything would work fine on the client-side with such configuration, as we are doing a lot of checks based on the serverBasePath

You are correct, this actually doesn't work. I verified this with nginx to show the problem. It's very apparent with just the login redirect. Kibana will include the serverBasePath in the redirect which doesn't work since the public address does not include that base path. Here is the config I tried:

# nginx.conf
server {
  listen 5610;

  server_name mydomain;

  location / {
    proxy_pass http://127.0.0.1:5601/base-path;
  }
}

$ yarn start --server.basePath=/base-path

$ curl mydomain:5610 -v
< HTTP/1.1 302 Found
< Server: nginx/1.19.3
< Date: Tue, 08 Dec 2020 17:30:39 GMT
< Content-Type: application/octet-stream
< Content-Length: 0
< Connection: keep-alive
< location: /base-path/login?next=%2Fbase-path%2F

I'm going to opt for leaving the validation as-is then, which verified that server.publicBaseUrl includes server.basePath. In the future, we could change how we do redirect to use the public base URL so that we could be more flexible, but I don't think this is a real need today.

pgayvallet

LGTM

pgayvallet · 2020-12-08T19:09:57Z

src/core/server/http/base_path_service.ts

-  constructor(serverBasePath: string = '') {
+  constructor(serverBasePath: string = '', publicBaseUrl?: string) {
    this.serverBasePath = serverBasePath;
+    this.publicBaseUrl = publicBaseUrl;
  }


I'm wondering if the integrity check between basePath and publicBaseUrl's path should be done here instead of src/core/server/http/http_config.ts. I guess by doing that in the config, we fail a little faster, but OTOH we can (programmatically) instantiate the service in an invalid state. Probably fine as it is right now.

I think for consistency it's best we do validation in a single place. If we want to make HttpConfig more robust, we should switch to a pattern where all validation is done in the constructor.

pgayvallet · 2020-12-08T19:14:29Z

src/core/server/http/http_config.ts

@@ -106,6 +108,17 @@ export const config = {
        if (!rawConfig.basePath && rawConfig.rewriteBasePath) {
          return 'cannot use [rewriteBasePath] when [basePath] is not specified';
        }
+
+        if (rawConfig.publicBaseUrl) {
+          const parsedUrl = url.parse(rawConfig.publicBaseUrl);


We don't need to try/catch the url.parse call because we are using schema.uri, right?

Yeah I'm relying on config-schema/Joi here implicitly.

pgayvallet · 2020-12-08T19:21:42Z

src/core/server/http/http_config.ts

+          if (parsedUrl.path !== (rawConfig.basePath ?? '/')) {
+            return `[publicBaseUrl] must contain the [basePath]: ${parsedUrl.path} !== ${rawConfig.basePath}`;
+          }


Does url.parse('http://myhost.com:4').path === '/'? I'm unsure, looking at the tests.

Yes, the path is / when there is no path in the input URL. I'll add a test to make this more clear / ensure it doesn't break

Actually I believe this covers this:

expect(() => httpSchema.validate({ publicBaseUrl: 'http://myhost.com' })).not.toThrow(); expect(() => httpSchema.validate({ publicBaseUrl: 'http://myhost.com/' })).not.toThrow();

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

…ase-url

kibanamachine · 2020-12-08T22:21:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9f5bfdc

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	46960	47720	+760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	548.1KB	548.3KB	+129.0B

History

💚 Build #92570 succeeded a4722a7
💔 Build #92532 failed 4ea6726
💔 Build #92490 failed 618ba16
💔 Build #92122 failed 1a24384

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

…k-field-to-hot-phase * 'master' of github.com:elastic/kibana: (429 commits) simplify popover open state logic (elastic#85379) [Logs UI][Metrics UI] Move actions to the kibana header (elastic#84648) [Search Source] Do not pick scripted fields if * provided (elastic#85133) [Search] Session SO polling (elastic#84225) [Transform] Replace legacy elasticsearch client (elastic#84932) [Uptime]Refactor header and action menu (elastic#83779) Fix agg select external link (elastic#85380) [ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled (elastic#85292) clear using keyboard (elastic#85042) [GS] add tag and dashboard suggestion results (elastic#85144) [ML] API integration tests - skip GetAnomaliesTableData Add ECS field for event.code. (elastic#85109) [Functional][TSVB] Wait for markdown textarea to be cleaned (elastic#85128) skip flaky suite (elastic#62060) skip flaky suite (elastic#85098) Bump highlight.js to v9.18.5 (elastic#84296) Add `server.publicBaseUrl` config (elastic#85075) [Alerting & Actions ] More debug logging (elastic#85149) [Security Solution][Case] Manual attach alert to a case (elastic#82996) Loosen UUID regex to accept uuidv1 or uuidv4 (elastic#85338) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts # x-pack/plugins/index_lifecycle_management/server/routes/api/policies/register_create_route.ts

Add server.publicBaseUrl config

1a24384

joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 4, 2020

joshdover requested a review from a team as a code owner December 4, 2020 21:10

joshdover requested a review from mikecote December 4, 2020 21:11

mikecote reviewed Dec 7, 2020

View reviewed changes

config/kibana.yml Show resolved Hide resolved

joshdover added 2 commits December 7, 2020 10:42

Fix lint + snapshot tests

74f7517

Merge remote-tracking branch 'upstream/master' into core/add-public-b…

618ba16

…ase-url

joshdover requested a review from a team as a code owner December 7, 2020 19:24

mikecote approved these changes Dec 7, 2020

View reviewed changes

joshdover added 3 commits December 7, 2020 14:19

Fix dashboard test snapshot

e00d70c

Merge remote-tracking branch 'upstream/master' into core/add-public-b…

4ea6726

…ase-url

Fix dashboard test snapshot

a4722a7

pgayvallet approved these changes Dec 8, 2020

View reviewed changes

joshdover added 2 commits December 8, 2020 13:41

Merge remote-tracking branch 'upstream/master' into core/add-public-b…

724b258

…ase-url

Add tests for hash, query, and auth exceptions

9f5bfdc

joshdover merged commit e116fce into elastic:master Dec 9, 2020

joshdover deleted the core/add-public-base-url branch December 9, 2020 00:02

joshdover mentioned this pull request Dec 9, 2020

[7.x] Add server.publicBaseUrl config (#85075) #85361

Merged

joshdover added a commit that referenced this pull request Dec 9, 2020

[7.x] Add server.publicBaseUrl config (#85075) (#85361)

5ea8281

jportner mentioned this pull request Nov 9, 2021

Limit authentication providers based on hostname #109525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `server.publicBaseUrl` config #85075

Add `server.publicBaseUrl` config #85075

joshdover commented Dec 4, 2020

elasticmachine commented Dec 4, 2020

mikecote left a comment

pgayvallet commented Dec 7, 2020

joshdover commented Dec 7, 2020

mikecote left a comment

pgayvallet commented Dec 8, 2020

joshdover commented Dec 8, 2020

pgayvallet left a comment

pgayvallet Dec 8, 2020

joshdover Dec 8, 2020

pgayvallet Dec 8, 2020

joshdover Dec 8, 2020

pgayvallet Dec 8, 2020

joshdover Dec 8, 2020

joshdover Dec 8, 2020

kibanamachine commented Dec 8, 2020

Add server.publicBaseUrl config #85075

Add server.publicBaseUrl config #85075

Conversation

joshdover commented Dec 4, 2020

Summary

Checklist

For maintainers

elasticmachine commented Dec 4, 2020

mikecote left a comment

Choose a reason for hiding this comment

pgayvallet commented Dec 7, 2020

joshdover commented Dec 7, 2020

mikecote left a comment

Choose a reason for hiding this comment

pgayvallet commented Dec 8, 2020

joshdover commented Dec 8, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Dec 8, 2020

Choose a reason for hiding this comment

joshdover Dec 8, 2020

Choose a reason for hiding this comment

pgayvallet Dec 8, 2020

Choose a reason for hiding this comment

joshdover Dec 8, 2020

Choose a reason for hiding this comment

pgayvallet Dec 8, 2020

Choose a reason for hiding this comment

joshdover Dec 8, 2020

Choose a reason for hiding this comment

joshdover Dec 8, 2020

Choose a reason for hiding this comment

kibanamachine commented Dec 8, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

Page load bundle

History

Add `server.publicBaseUrl` config #85075

Add `server.publicBaseUrl` config #85075