Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Elastic Tile Service and configurable leaflet providers #7724

Merged
merged 15 commits into from
Jul 20, 2016
Merged

Elastic Tile Service and configurable leaflet providers #7724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
58844a0
[viz] Allow for configurable map urls
jbudz Jul 12, 2016
0b9388c
Adds every config option from Leaflet
pickypg Jul 13, 2016
b1f8ac8
Removed confusing config options
pickypg Jul 13, 2016
c670a1c
[vis] Don't rewrite markdown
jbudz Jul 13, 2016
7c8ec75
[config] Update default tile link
jbudz Jul 13, 2016
6d8f803
Change bounds from object to GeoJSON (long, lat) array style
pickypg Jul 13, 2016
b762949
[config] Set max zoom to 7
jbudz Jul 13, 2016
c3d203f
[tests] Cleanup schema data
jbudz Jul 13, 2016
3c1339e
[vis] Bound tilemap zoom
jbudz Jul 13, 2016
41fbb06
Round out support for the initial Elastic Tile Service
epixa Jul 15, 2016
6494caa
Made minZoom in test fixture consistent with reality
epixa Jul 15, 2016
c74041c
Correctly check for enabled wms to determine zoom
epixa Jul 15, 2016
ca5a968
Remove duplicate wms conditional in tilemap createmap
epixa Jul 15, 2016
2feac11
Remove optional joi references in tilemap config
epixa Jul 19, 2016
2862f94
Re-disable scroll wheel zoom on maps
epixa Jul 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/cli/serve/legacy_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ export const legacySettings = {
request_timeout: 'elasticsearch.requestTimeout',
shard_timeout: 'elasticsearch.shardTimeout',
startup_timeout: 'elasticsearch.startupTimeout',
tilemap_url: 'tilemap.url',
tilemap_min_zoom: 'tilemap.options.minZoom',
tilemap_max_zoom: 'tilemap.options.maxZoom',
tilemap_attribution: 'tilemap.options.attribution',
tilemap_subdomains: 'tilemap.options.subdomains',
verify_ssl: 'elasticsearch.ssl.verify',
};

Expand Down
4 changes: 2 additions & 2 deletions src/core_plugins/kibana/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ module.exports = function (kibana) {

injectVars: function (server, options) {
let config = server.config();

return {
kbnDefaultAppId: config.get('kibana.defaultAppId')
kbnDefaultAppId: config.get('kibana.defaultAppId'),
tilemap: config.get('tilemap')
};
},
},
Expand Down
10 changes: 9 additions & 1 deletion src/core_plugins/tests_bundle/tests_entry_template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,15 @@ window.__KBN__ = {
kbnIndex: '.kibana',
esShardTimeout: 1500,
esApiVersion: '2.0',
esRequestTimeout: '300000'
esRequestTimeout: '300000',
tilemap: {
url: 'https://tiles.elastic.co/v1/default/{z}/{x}/{y}.png?my_app_name=kibana&my_app_version=1.2.3&elastic_tile_service_tos=agree',
options: {
minZoom: 1,
maxZoom: 7,
attribution: '© [Elastic Tile Service](https://www.elastic.co/elastic_tile_service)'
}
}
},
uiSettings: {
defaults: ${JSON.stringify(env.defaultUiSettings, null, 2).split('\n').join('\n ')},
Expand Down
17 changes: 17 additions & 0 deletions src/server/config/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import os from 'os';
import { fromRoot } from '../../utils';
import { getData } from '../path';

import pkg from '../../../src/utils/package_json';

module.exports = () => Joi.object({
pkg: Joi.object({
version: Joi.string().default(Joi.ref('$version')),
Expand Down Expand Up @@ -130,6 +132,21 @@ module.exports = () => Joi.object({

status: Joi.object({
allowAnonymous: Joi.boolean().default(false)
}).default(),

tilemap: Joi.object({
url: Joi.string().default(`https://tiles.elastic.co/v1/default/{z}/{x}/{y}.png?my_app_name=kibana&my_app_version=${pkg.version}&elastic_tile_service_tos=agree`),
options: Joi.object({
attribution: Joi.string().default('© [Elastic Tile Service](https://www.elastic.co/elastic-tile-service)'),
minZoom: Joi.number().min(1, 'Must not be less than 1').default(1),
maxZoom: Joi.number().default(7),
tileSize: Joi.number(),
subdomains: Joi.array().items(Joi.string()).single(),
errorTileUrl: Joi.string().uri(),
tms: Joi.boolean(),
reuseTiles: Joi.boolean(),
bounds: Joi.array().items(Joi.array().items(Joi.number()).min(2).required()).min(2)
}).default()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's actually no need to set these as optional, keys are optional by default in Joi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's on me, but I preferred the readability because I was mixing it with ones that were defined with default(...) and most did not.

}).default()

}).default();
4 changes: 4 additions & 0 deletions src/ui/public/vislib/styles/_tilemap.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@
.leaflet-control-attribution {
background-color: @tilemap-leaflet-footer-bg !important;
color: @tilemap-leaflet-footer-color !important;

p {
display: inline;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running attribution through markdown wraps everything in a p block, causes linebreak

}

.leaflet-left {
Expand Down
30 changes: 19 additions & 11 deletions src/ui/public/vislib/visualizations/_map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
import _ from 'lodash';
import $ from 'jquery';
import L from 'leaflet';
import marked from 'marked';
marked.setOptions({
gfm: true, // Github-flavored markdown
sanitize: true // Sanitize HTML tags
});

import VislibVisualizationsMarkerTypesScaledCirclesProvider from 'ui/vislib/visualizations/marker_types/scaled_circles';
import VislibVisualizationsMarkerTypesShadedCirclesProvider from 'ui/vislib/visualizations/marker_types/shaded_circles';
import VislibVisualizationsMarkerTypesGeohashGridProvider from 'ui/vislib/visualizations/marker_types/geohash_grid';
import VislibVisualizationsMarkerTypesHeatmapProvider from 'ui/vislib/visualizations/marker_types/heatmap';
export default function MapFactory(Private) {
export default function MapFactory(Private, tilemap) {

let defaultMapZoom = 2;
let defaultMapCenter = [15, 5];
let defaultMarkerType = 'Scaled Circle Markers';

let tilemapOptions = tilemap.options;
let attribution = marked(tilemapOptions.attribution);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why markdown instead of HTML? This seems like it would be confusing for users familiar with Leaflet, since Leaflet's attribution option takes HTML. XSS shouldn't be an issue since this is only configurable in kibana.yml right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but now it's already in 4.1 and 4.5 FYI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, yes. I tend to err on the side of caution with anything like this - what if we do per visualization tilemap layers on the client in the future, this helps protect future changes from people who may not be as familiar with the code, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I just checked the tags and I see that you're right. Did that actually go out in the releases? I swear I remember copy/pasting the html from the leaflet demos while testing the release candidates and it worked fine. Maybe it was added between sets of release candidates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it went out in the releases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, in that case I think we should keep it as is for now. We shouldn't change the existing API in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, simple html is valid markdown, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spalger no, because the sanitize option is enabled.

When I used the string

'Map data: &copy; <a href="http://www.openstreetmap.org/copyright">OpenStreetMap</a>, <a href="http://viewfinderpanoramas.org">SRTM</a> | Map style: &copy; <a href="https://opentopomap.org">OpenTopoMap</a> (<a href="https://creativecommons.org/licenses/by-sa/3.0/">CC-BY-SA</a>)'

The output on the page was the literal string, html tags and all.


let mapTiles = {
url: 'https://otile{s}-s.mqcdn.com/tiles/1.0.0/map/{z}/{x}/{y}.jpeg',
options: {
attribution: 'Tiles by <a href="http://www.mapquest.com/">MapQuest</a> &mdash; ' +
'Map data &copy; <a href="http://openstreetmap.org">OpenStreetMap</a> contributors, ' +
'<a href="http://creativecommons.org/licenses/by-sa/2.0/">CC-BY-SA</a>',
subdomains: '1234'
}
url: tilemap.url,
options: _.assign({}, tilemapOptions, { attribution })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it matters, but it seems like subdomains previously defaulted to '1234' whereas now it defaults to nothing (based on the schema)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye, but that is actually what we want in this case. We don't use subdomains for our tile service, so the correct new default is empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note: subdomains isn't even used unless the URL itself has a {s} parameter like the one we've replaced (and Leaflet itself defaults the subdomains value to 'abc' when it's used, but not supplied).

};

let markerTypes = {
Expand Down Expand Up @@ -47,13 +51,13 @@ export default function MapFactory(Private) {
this._valueFormatter = params.valueFormatter || _.identity;
this._tooltipFormatter = params.tooltipFormatter || _.identity;
this._geoJson = _.get(this._chartData, 'geoJson');
this._mapZoom = params.zoom || defaultMapZoom;
this._mapZoom = Math.max(Math.min(params.zoom || defaultMapZoom, tilemapOptions.maxZoom), tilemapOptions.minZoom);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WMS should reinterpret this value with its view of maxZoom and minZoom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 4.5 the saved zoom was getting pulled out of the geoJson object. Did we find out params is the correct place to get it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bargs No, master just handles the properties differently because of a change that recently went in to make zoom levels handled through persistent state.

this._mapCenter = params.center || defaultMapCenter;
this._attr = params.attr || {};

let mapOptions = {
minZoom: 1,
maxZoom: 18,
minZoom: tilemapOptions.minZoom,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this go away?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question. @jbudz Do you remember?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better question might be why was it there in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why it is there in the first place, but this isn't the right PR in which to change the behavior, especially since that configuration is being passed in 4.x. I'll add it back in for now.

@pickypg If you think it should be removed, feel free add an issue for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed @epixa. I created #7778 to make that happen once this is committed.

maxZoom: tilemapOptions.maxZoom,
noWrap: true,
maxBounds: L.latLngBounds([-90, -220], [90, 220]),
scrollWheelZoom: false,
Expand Down Expand Up @@ -277,6 +281,10 @@ export default function MapFactory(Private) {

// add map tiles layer, using the mapTiles object settings
if (this._attr.wms && this._attr.wms.enabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement should just be placed with the one right below doing the exact same check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

_.assign(mapOptions, {
minZoom: 1,
maxZoom: 18
});
this._tileLayer = L.tileLayer.wms(this._attr.wms.url, this._attr.wms.options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than override mapOptions, I wonder if this could go in this._attr.wms.options? It seems like this._attr.wms is where all the other wms overrides live.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't actually used by tileLayer.wms, it's used further down in L.map.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, after debugging I see now that those are two totally different option objects.

} else {
this._tileLayer = L.tileLayer(mapTiles.url, mapTiles.options);
Expand Down