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.

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

[Maps] Fix proxy handling issues #71182

Merged

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Jul 9, 2020

Resolves #67593. There were a few things preventing EMS proxies from functioning correctly:

  • URL construction in maps/public/meta.ts used relative paths which resolved to the incorrect final URL. They also lacked a prepended basepath which is necessary for them to work correctly in a dev environment although they might work fine in production.
  • The xpack.maps config was being passed to routes but they require values from the maps_legacy (map) config. This is admittedly confusing (related [Maps] Decouple maps and maps_legacy config dependencies #64810)
  • Added validation for several routes. This isn't always necessary unless the logic in the route handler leverages query or params vars or alternatively some validation exists but different params/queries are passed. We had a couple of routes involved in proxying that met these criteria.
  • Also did some minor cleanup while in routes. We had some residual logic testing for the presence of params, etc. Validation already covers this.

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 9, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@kindsun kindsun marked this pull request as ready for review July 13, 2020 16:04
@kindsun kindsun requested a review from a team as a code owner July 13, 2020 16:04
@elasticmachine
Copy link
Contributor

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

@kindsun kindsun requested review from nickpeihl and jsanz July 13, 2020 16:04
Copy link
Member

@jsanz jsanz left a comment

Choose a reason for hiding this comment

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

checked out locally and tested both with and without the proxy enabled and everything seems to work fine on the functional side 👍

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm. code review and tested in firefox

elastic_tile_service_tos: schema.maybe(schema.string()),
my_app_name: schema.maybe(schema.string()),
my_app_version: schema.maybe(schema.string()),
license: schema.maybe(schema.string()),
Copy link
Member

Choose a reason for hiding this comment

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

nit, but up to you really. we repeat some of these schemas such as elastic_tile_service_tos: schema.maybe(schema.string()), several times in this file. would it be feasible and easy to avoid duplicating these schemas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know what you mean, it does get a little repetitive. As far as I can tell, there's not an easy way to create reusable validation that we can also build upon (i.e.- add more validation where needed) unless we use a custom validator that does some base validation such as checking elastic_tile_service_tos: schema.maybe(schema.string()) etc. before doing any additional validation required by the calling route. tbh I'm still not sure that's the right path either so kicking that can down the road for now to get this fix in.

@kindsun kindsun merged commit d7a679b into elastic:master Jul 13, 2020
@kindsun kindsun deleted the fix-proxy-handling-and-meta-url-construction branch July 13, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] EMS proxy not working
5 participants