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

Upgrade Hapi in legacy platform to v17 #21707

Merged
merged 123 commits into from
Oct 25, 2018
Merged

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Aug 6, 2018

Fixes #13802
Fixes #21140

Summary of changes

  • Removed even-better fork of good
  • Updated all route handlers to use async/await rather than the reply callback
  • Updated changes to server.auth calls
  • Upgrade hapi, joi, wreck, h2o2, hapi-auth-cookie, inert, vision, boom
  • Added new validation failAction handler that returns errors similar to Hapi v14
  • Changes to how we use exports from joi
  • Reimplemented monitoring built on good. This time around, I'm going to push to get it accepted into the mainline package so we don't have to maintain a fork. PR here: Allow reconfiguring after monitor is started hapijs/good#593
  • Made subtle changes to how security plugin interacts with Hapi

Risks

As with any major upgrade, this PR has risks for breakage across the entire application. While I have made sure to pay attention to all these areas, it's possible I've missed things. Areas we may want to focus on in QA:

  • Validation errors. Any features that depend on the backend telling us that the frontend sent valid data should be checked. In particular how the error message is displayed may be broken in some areas. I believe I was able to get this to be exactly the same before, but it's possible some subtle thing was missed.
  • Authentication. This upgrade changes how we tell Hapi who the authenticated user is. We should validate that nothing about this behavior is wrong.
  • Logging. I have upgraded our core logging components in the legacy platform. I don't anticipate any major problems here.
  • Kibana Monitoring. For similar reasons, our monitoring of Kibana itself may have changed slightly. I have done my best to verify that we're sending the same data to ES for kibana's internal monitoring, but it's possible it's not quite right or misbehaves in bad conditions.

Potential Areas for Improvement

  • In order to have the same validation errors return from our backend as we did in Hapi v14, we have to install a default validation failAction handler. This means that any tests that don't set up their test server with this handler will not have the same validation responses as the real environment does. It would be a nice improvement if we had a single test server fixture that we used everywhere.

Release notes

release-note: The Hapi framework has been upgraded from v14.2.0 to v17.5.3. There are a number of breaking changes affecting plugins that register custom backend routes. The reply callback interface for supplying responses has been replaced with an async/await interface and a response toolkit. More details in the Hapi upgrade guide

Dev Docs

Upgraded Hapi framework for plugin backend endpoints

The Hapi framework has been upgraded from v14.2.0 to v17.5.3. There are a number of breaking changes affecting plugins that register custom bacekend routes. This does not affect plugins that do not have backend routes.

The reply callback interface for supplying responses has been replaced with an async/await interface and a "response toolkit". Example:

Before 6.5

server.route({
  path: '/myroute',
  handler(req, reply) {
    getSomeData().then((data) => reply(data));
  }
});

server.route({
  path: '/myredirect',
  handler(req, reply) {
    reply.redirect('/otherroute');
  }
});

After 6.5

server.route({
  path: '/myroute',
  async handler(req, h) {
    return await getSomeData();
  }
});

server.route({
  path: '/myredirect',
  handler(req, h) {
    return h.redirect('/otherroute');
  }
});

More details can be found in the Hapi upgrade guide.

@joshdover joshdover added the WIP Work in progress label Aug 6, 2018
kbnServer.server.log(['info', 'config'], 'Reloading logging configuration due to SIGHUP.');
await kbnServer.applyLoggingConfiguration(config);
kbnServer.server.log(['info', 'config'], 'Reloaded logging configuration due to SIGHUP.');
// kbnServer.server.log(['info', 'config'], 'Reloading logging configuration due to SIGHUP.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, this is all coming back to me. We need to get off our fork at bevacqua/even-better@4342ed5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I got some good details from Court on this. Going to go with getting everything working and then re-implement the things that depend on good with either a new version or something else.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover added v6.6.0 and removed v6.5.0 labels Oct 22, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover merged commit 27e5406 into elastic:master Oct 25, 2018
@joshdover joshdover deleted the hapi-upgrade branch October 25, 2018 21:01
joshdover added a commit to joshdover/kibana that referenced this pull request Oct 25, 2018
* Disable even-better monitoring

* Upgrade to Hapi v15

* Upgrade to Hapi v16

* Handle optional req params correctly

* Update http and kbnServer

* Get mocha tests passing

* Convert `reply` usages [wip]

* Fix Joi and Plugin incompatibilities

* Get server up and running

* Get basic logging working

* Fix optimizer

* Fix recent route handlers

* Various fixes

* Fix recent routes

* Upgrade wreck for async/await

* Fix mocha tests

* Fix joi issues

* Fix xpack jest tests

* Fix recent routes

* Fix tests

* Fix index setup

* Decouple monitoring stats collection from good plugin

* Update reload logging test to work

* Reimplement logging with updated good plugin

* Fix unit tests

* Fix getConnections back

* Make LegacyLoggingServer compatible with Hapi v17

* Update joi types

* Fix x-pack unit tests

* Remove stray debugger

* Remove hapi-compat

* Fix API integrations

* Upgrade boom

* Fix security plugin

* Misc fixes

* bump

* Fix licensePreRoutingFactory

* Fix failing integration tests

* Remove unnecessary test change

* Remove hapi-latest package

* fx

* Various cleanup

* Fix race condition in oppsy events

* Use elastic/good fork

* Fix boom.wrap and hapi-latest changes

* Simplify LegacyLoggingServer updates

* package.json cleanup + test fix

* yarn.lock cleanup

* Change good tag

* Fixes

* Change return err -> throw err in routes

* Fix await returns

* Fix new load_data test

* Make cookie security flags consistent

* tmp doc

* Fix types

* Fix tests

* Upgrade canvas plugin

* Move good package to published @elastic/good one

* Fix SO test

* Fix logging reloading

* Update APM apis

* Fix error logging

* Fix logging test

* Convert spaces plugin

* Add validation error shim

* Remove 7.0 release notes

* Await renderApp

* Fix ccr routes

* Prevent header popovers from scrolling with page content (elastic#23850)

* Fix spaces test

* new yarn.lock-s

* Fix spaces tests

* Remove h2o2-latest

* Fix @types/hapi

* Upgrade InfraOps plugin

* Fix package.json

* Add back isSameSite: false

* Upgrade beats_management plugin

* Update snapshot

* Fix InfraOps

* Upgrade kql_telemetry

* Merge upstream/master

* Upgrade apm and ml

* Put snapshot test back

* Fx beats

* Upgrade rollups

* Update boom usages in new plugins
joshdover added a commit that referenced this pull request Oct 26, 2018
* Upgrade Hapi in legacy platform to v17 (#21707)

* Disable even-better monitoring

* Upgrade to Hapi v15

* Upgrade to Hapi v16

* Handle optional req params correctly

* Update http and kbnServer

* Get mocha tests passing

* Convert `reply` usages [wip]

* Fix Joi and Plugin incompatibilities

* Get server up and running

* Get basic logging working

* Fix optimizer

* Fix recent route handlers

* Various fixes

* Fix recent routes

* Upgrade wreck for async/await

* Fix mocha tests

* Fix joi issues

* Fix xpack jest tests

* Fix recent routes

* Fix tests

* Fix index setup

* Decouple monitoring stats collection from good plugin

* Update reload logging test to work

* Reimplement logging with updated good plugin

* Fix unit tests

* Fix getConnections back

* Make LegacyLoggingServer compatible with Hapi v17

* Update joi types

* Fix x-pack unit tests

* Remove stray debugger

* Remove hapi-compat

* Fix API integrations

* Upgrade boom

* Fix security plugin

* Misc fixes

* bump

* Fix licensePreRoutingFactory

* Fix failing integration tests

* Remove unnecessary test change

* Remove hapi-latest package

* fx

* Various cleanup

* Fix race condition in oppsy events

* Use elastic/good fork

* Fix boom.wrap and hapi-latest changes

* Simplify LegacyLoggingServer updates

* package.json cleanup + test fix

* yarn.lock cleanup

* Change good tag

* Fixes

* Change return err -> throw err in routes

* Fix await returns

* Fix new load_data test

* Make cookie security flags consistent

* tmp doc

* Fix types

* Fix tests

* Upgrade canvas plugin

* Move good package to published @elastic/good one

* Fix SO test

* Fix logging reloading

* Update APM apis

* Fix error logging

* Fix logging test

* Convert spaces plugin

* Add validation error shim

* Remove 7.0 release notes

* Await renderApp

* Fix ccr routes

* Prevent header popovers from scrolling with page content (#23850)

* Fix spaces test

* new yarn.lock-s

* Fix spaces tests

* Remove h2o2-latest

* Fix @types/hapi

* Upgrade InfraOps plugin

* Fix package.json

* Add back isSameSite: false

* Upgrade beats_management plugin

* Update snapshot

* Fix InfraOps

* Upgrade kql_telemetry

* Merge upstream/master

* Upgrade apm and ml

* Put snapshot test back

* Fx beats

* Upgrade rollups

* Update boom usages in new plugins

* Update url shortener

* Don't throw errors in optimizer (#24660)
// tslint:disable-next-line
console.error(err.stack);
// @ts-ignore
reply(Boom.wrap(err, err.statusCode || 400));
Boom.boomify(err, { statusCode: err.statusCode || 400 });
Copy link
Member

Choose a reason for hiding this comment

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

@joshdover Minor thing: this should be returned or thrown. We've created an issue for it here: #24844

We'll fix the APM instances (if there are more) but just a heads up if this is also the case for other plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:breaking release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.6.0 v7.0.0
Projects
None yet