[Maps] Always initialize routes on server-startup #84806

thomasneirynck · 2020-12-02T18:04:10Z

Summary

Routes would be initialized only on the first subscription-event from the license-observable. A subscribe-event on an Observable provides no guarantees this will fire after startup of the Maps plugin.

The reason route-init was delayed until license-information became available was because the EMSClient-library needs a license-id.

In certain scenarios, routes would not get initialized, causing 404s when Maps attempts to make a backend call.

This would manifest in the Maps-app in subtle ways. Starting in ssl-mode would be especially sensitive to this issue

adding document layers would raise user-role toast-warnings
uploading geojson would raise user-role toast-warnings
vector tile rendering would fail silently
with proxying EMS in maps, default baselayer would fail silently / could not add new EMS layers

Solution:

Routes will always get initialized on plugin startup. This PR adds a callback to retrieve the license-id for the EMSClient.

A minor bonus of this change is that the license-changes will cause the EMSClient to be recreated. This only applies to the proxying of EMS-request, and this feature will be deprecated in 8.0 regardless (#82132), so impact of this is minor.

For maintainers

This was checked for breaking API changes and was labeled appropriately

thomasneirynck · 2020-12-02T18:05:11Z

x-pack/plugins/maps/server/plugin.ts

      const enterprise = license.check(APP_ID, 'enterprise');
      isEnterprisePlus = enterprise.state === 'valid';
-
-      if (basic.state === 'valid' && !routesInitialized) {


checking for validity is not necessary. Other plugins do not check license-validity before registering the routes.

Might be carryover from legacy patterns? Thanks for the clean up

elasticmachine · 2020-12-02T18:10:03Z

Pinging @elastic/kibana-gis (Team:Geo)

x-pack/plugins/maps/server/routes.js

nreese · 2020-12-02T20:23:21Z

x-pack/plugins/maps/server/routes.js

+    }
+
+    lastLicenseId = currentLicenseId;
+    if (emsSettings.isIncludeElasticMapsService()) {


Should this include a check for lastLicenseId being undefined? What happens if getLicenseId returns undefined?

EMS has a license validation on the backend as well. This will fallback to as-if OSS EMS would be used.

x-pack/plugins/maps/server/plugin.ts

kindsun

Thanks so much for fixing! 🙇 Changes make sense and works as expected with ssl enabled

code review
tested locally in chrome

nreese

Thanks for digging into this problem and fixing.

LGTM
code review

kibanamachine · 2020-12-02T23:36:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 53d409c

Metrics [docs]

✅ unchanged

History

💚 Build #91476 succeeded 2a93e26
💔 Build #91465 failed f02cacb

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

* master: (40 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...

* master: (236 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...

…overy-action-group * upstream/master: (48 commits) [Lens] accessibility screen reader issues (elastic#84395) [Logs UI] Fetch single log entries via a search strategy (elastic#81710) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) ...

init

5006a5f

thomasneirynck commented Dec 2, 2020

View reviewed changes

remove log

f02cacb

thomasneirynck added v7.11.0 v8.0.0 release_note:fix bug Fixes for quality problems that affect the customer experience [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Dec 2, 2020

type fix

2a93e26

lukeelmers mentioned this pull request Dec 2, 2020

[data.search.searchSource] Update SearchSource to use Fields API. #82383

Merged

thomasneirynck requested a review from kindsun December 2, 2020 20:04

nreese reviewed Dec 2, 2020

View reviewed changes

kindsun reviewed Dec 2, 2020

View reviewed changes

x-pack/plugins/maps/server/plugin.ts Outdated Show resolved Hide resolved

kindsun approved these changes Dec 2, 2020

View reviewed changes

feedback

53d409c

nreese approved these changes Dec 2, 2020

View reviewed changes

thomasneirynck changed the title ~~[Maps] Initialize routes on server-startup~~ [Maps] Always initialize routes on server-startup Dec 2, 2020

thomasneirynck merged commit 65cbe4c into elastic:master Dec 2, 2020

thomasneirynck mentioned this pull request Dec 2, 2020

[7.x] [Maps] Always initialize routes on server-startup (#84806) #84851

Merged

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Dec 2, 2020

[Maps] Always initialize routes on server-startup (elastic#84806)

0533185

thomasneirynck added a commit that referenced this pull request Dec 3, 2020

[Maps] Always initialize routes on server-startup (#84806) (#84851)

ee16ac2

thomasneirynck mentioned this pull request Dec 14, 2020

[Maps] Should make sure license check completes at startup #85863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Maps] Always initialize routes on server-startup #84806

[Maps] Always initialize routes on server-startup #84806

thomasneirynck commented Dec 2, 2020 •

edited

Loading

thomasneirynck Dec 2, 2020

kindsun Dec 2, 2020

elasticmachine commented Dec 2, 2020

nreese Dec 2, 2020

thomasneirynck Dec 2, 2020

kindsun left a comment

nreese left a comment

kibanamachine commented Dec 2, 2020

[Maps] Always initialize routes on server-startup #84806

[Maps] Always initialize routes on server-startup #84806

Conversation

thomasneirynck commented Dec 2, 2020 • edited Loading

Summary

For maintainers

thomasneirynck Dec 2, 2020

Choose a reason for hiding this comment

kindsun Dec 2, 2020

Choose a reason for hiding this comment

elasticmachine commented Dec 2, 2020

nreese Dec 2, 2020

Choose a reason for hiding this comment

thomasneirynck Dec 2, 2020

Choose a reason for hiding this comment

kindsun left a comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 2, 2020

💚 Build Succeeded

Metrics [docs]

History

thomasneirynck commented Dec 2, 2020 •

edited

Loading