Skip to content

Upgrade Hapi in legacy platform to v17#21707

Merged
joshdover merged 123 commits intoelastic:masterfrom
joshdover:hapi-upgrade
Oct 25, 2018
Merged

Upgrade Hapi in legacy platform to v17#21707
joshdover merged 123 commits intoelastic:masterfrom
joshdover:hapi-upgrade

Conversation

@joshdover
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Contributor

💔 Build Failed

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

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

console.error(err.stack);
// @ts-ignore
reply(Boom.wrap(err, err.statusCode || 400));
Boom.boomify(err, { statusCode: err.statusCode || 400 });
Copy link
Copy Markdown
Contributor

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 Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v6.6.0 v7.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-enable Server logging configuration › should be reloadable via SIGHUP process signaling Upgrade hapi to v17