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

Add http2 support for Kibana server #183465

Merged
merged 41 commits into from
Jun 3, 2024

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented May 15, 2024

Summary

Part of #7104

Add support for http2 to the Kibana server. http2 can be enabled by setting server.protocol: http2 in the Kibana config file.

Note: by default, enabling http2 requires a valid h2c configuration, meaning that it can only run over HTTPS with TLS1.2+

## kibana.yaml
server.protocol: http2
server.ssl.enabled: true
server.ssl.key: path/to/key
server.ssl.certificate: path/my/cerf

What is this PR doing

Add HTTP2 support for the Kibana server

- Plug http2 to the Kibana server

Even if HAPI was never officially updated to really support HTTP2, node's http/https/http2 modules are compatible enough to be able to just instantiate an http2 server/listener and provide it to HAPI "as a plain https listener". There were some tweaks to do (mostly silencing a few warnings that HAPI was causing by sending http2-illegal headers such as Connection), but overall, it went smoothly.

- Add config validation

By default, Kibana will require a valid h2c configuration to accept enabling http2. It means that TLS must be enabled and that TLS1.2+ should at least be in the list of supported SSL protocols (server.ssl.supportedProtocols). Note that default value of this setting includes TLS1.2 and 1.3.

- Add escape hatch to run h2 without h2c

In some situations, it may be required to enable http2 without a valid h2c configuration. Kibana supports it, by setting server.http2.allowUnsecure to true.

(Note, however, that if http2 is enabled without TLS, ALPN protocol negotiation won't work, meaning that most http2 agents/clients will fail connecting unless they're explictly configured to use http2.)

Add documentation about this new feature

- Update the user-facing doc about this new server.protocol setting

Update the user-facing Kibana settings documentation to include this http.protocol setting (and refer to server.http2.allowUnsecure)

Note: this setting, and this feature, are considered as experimental

Adapt our dev tooling to support running Kibana with http2 enabled

- Add a --http2 flag to the dev CLI

Enabling this flag will add the proper configuration settings to run Kibana with http2 enabled in an (almost) valid h2c configutation.

Note: when using this flag, even if listening on the same port, the Kibana server will be accessible over https, meaning that you need to use https in your browser to access it. Aka http://localhost:5601 won't work, you need to use https://localhost:5601. Also, we're using the self-signed dev certificates, meaning that you must go though the scary warning of your browser

- Implement an http2-compatible base-path proxy

The current base path proxy is based on hapi and hapi/h2o2. I tried for a bunch hours trying to hack around to make it work with http2 proxying, but ultimately gave up and implemented a new version from scratch.

Note that with some additional efforts, this new http2 basepath proxy could probably fully replace the existing one and be used for both http1 and http2 traffic, but it's an optimization / refactoring that did not feel required for this PR.

Adapt the FTR to run suites against http2

- Add support to run FTR test suite against an h2c-enabled Kibana

Note that with ALPN, clients using http1 should be (and are) able to communicate with http2 Kibana, given h2c/alpn allows protocol negitiation. So adapting our FTR tooling was not really about making it work with http2 (which worked out of the box), but making it work with the self signed certifcates we use for https on dev mode

Note that I'm not a big fan of what I had to do, however, realistically this was the only possible approach if we want to run arbitrary test suites with TLS/HTTP2 enabled without massively changing our FTR setup.

Operations and QA, feel free to chime in there, as this is your territory.

- Change some FTR test suites to run against an HTTP2-enabled server

I added a quick configureHTTP2 helper function to take any "final" FTR suite config and mutate it to enable http2. I then enabled it on a few suites locally, to make sure the suites were passing correctly.

I kept two suites running with http2 enabled:

  • the console oss functional tests
  • the home oss functional tests

We could possibly enable it for more, but we need to figure out what kind of strategy we want on that matter (see below)

What is this pull request NOT doing

- Making sure everything works when HTTP2 is enabled

I navigated the applications quite a bit, and did not see anything broken, however I obviously wasn't able to do a full coverage. Also, the self-signed certificate was a huge pain to detect issues really caused by http2 compared to issues because the local setup isn't valid h2c.

In theory though (famous last words) anything not doing http/1.1 specific hacks such as bfetch should work fine with http2, given that even if using non-http2 clients, ALPN should just allow to fallback to http/1.x (this part was tested)

- Enabling HTTP2 by default

PR isn't doing it for obvious reasons.

- Enabling HTTP2 for all FTR suites

First of all, it's not that easy, because it requires adapting various parts of the config (and even some var env...), and we don't have any proper way to override config "at the end". For instance, if you add the http2 config on a top level config (e.g. the oss functional one that is reuse by the whole world - learned the hard way), it won't work because higher-level configs redefined (and override) the browser part of the config, loosing the settings added to run the browser in insecure mode.

Secondly, I'm not sure we really need to run that many suites with http2 enabled. I learned working on that PR that we only have like one suite where https is enabled for the Kibana server, and I feel like it could be fine to have the same for http2. In theory it's just a protocol change, unless parts of our apps (e.g. bfetch) are doing things that are specific to http/1.1, switching to http2 should be an implementation detail.

But I'd love to get @elastic/kibana-operations and @elastic/appex-qa opinion on that one, given they have more expertise than I do on that area.

  • Running performances tests

We should absolutely run perf testing between http/1.1 over https and http/2, to make sure that it goes into the right directly (at least in term of user perceived speed), but I did not do it in the scope of this PR (and @dmlemeshko is on PTO so... 😅)

Release Note

Add support for http2 to the Kibana server. http2 can be enabled by setting server.protocol: http2 in the Kibana config file.

Note: by default, enabling http2 requires a valid h2c configuration, meaning that it can only run over HTTPS with TLS1.2+

Please refer to the Kibana config documentation for more details.

@pgayvallet
Copy link
Contributor Author

/ci

1 similar comment
@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet pgayvallet force-pushed the kbn-7104-http2-reborn branch from 03be0a4 to 8b7f775 Compare May 17, 2024 07:51
pgayvallet added a commit that referenced this pull request May 21, 2024
## Summary

Part of #7104

Prepare the work for `http2` support by introducing the `httpVersion`
and `protocol` fields to the `KibanaRequest` type and implementation.

Proper handling of h2 protocol for those fields will be added in the PR
implementing http2 (#183465)
pgayvallet added a commit that referenced this pull request May 21, 2024
## Summary

Related to #7104
Adapted from #183465

For `http2` support, we will have to change the way we configure the
HAPI server to manually provide the listener instead of passing down the
options for HAPI to create it.

This PR prepares that work, by creating the `http` or `https` (`tls`)
listener and passing it when creating the HAPI server instead of just
passing the `tls` options.

**Note:** no integration tests were added, because we already have the
right coverage for both tls and non-tls mode, so any change of behavior
introduced by the PR should be detectable by them.
@pgayvallet pgayvallet added release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.15.0 labels May 23, 2024
@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet
Copy link
Contributor Author

/ci

@pgayvallet
Copy link
Contributor Author

/ci

Copy link
Contributor Author

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Self-review (quite verbose to explain the reasonning behind most of the PR's changes)

Comment on lines +1626 to +1627
"http2-proxy": "^5.0.53",
"http2-wrapper": "^2.2.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dev deps - only used by the new http2 basepathproxy server in dev setup

Comment on lines +269 to +276
if (kibanaRequest.protocol === 'http2' && kibanaResponse.options.headers) {
kibanaResponse.options.headers = stripIllegalHttp2Headers({
headers: kibanaResponse.options.headers,
isDev: this.options.isDev ?? false,
logger: this.log,
requestContext: `${request.route.method} ${request.route.path}`,
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Illegal http2 headers (see stripIllegalHttp2Headers for the list) are not only causing warnings, they can also just terminate the Kibana process.

That's why we're removing it in the handler wrapper, and we log a warning in development mode so that developers can be informed and remove it from their handler.

Comment on lines +122 to +124
protocol: schema.oneOf([schema.literal('http1'), schema.literal('http2')], {
defaultValue: 'http1',
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to go with server.http2.enabled: true/false, because it would then be a pain for when we might decide to enable http3 (assuming nodejs fully supports it at some point), so I went with server.protocol: http1 | http2 instead.

Note that there are many different concepts of protocols in http, given http/https are protocols, http1/http2 are protocols of a different kind... I thought of server.httpVersion: 1/2 but I didn't like it either... Anyway, if anyone thinks its could be misleading and have a better suggestion than the one I chose, please say so.

import type { BasePathProxyServer, BasePathProxyServerOptions } from './types';
import { getRandomBasePath } from './utils';

export class Http2BasePathProxyServer implements BasePathProxyServer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notes about the http2 base path proxy:

  • It doesn't support the /{oldBasePath}/{kbnPath*} route that the http1 version has
  • It doesn't support the /__UNSAFE_bypassBasePath/{kbnPath*} route that the http1 version has

(but I'm not sure anyone ever used those features especially the last one, so it didn't feel mandatory to implement again? Please tell me if I'm wrong)

Copy link
Member

@jbudz jbudz May 29, 2024

Choose a reason for hiding this comment

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

I see a few references to developers using __UNSAFE_bypassBasePath in scripts to check the status page and use es_archiver, but nothing recent or frequent. Seems fine, and http1 is still an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, If we discover we really need the __UNSAFE_bypassBasePath, it could easily be added later

},
} as AutoRequestOptions;

const proxyReq = await http2.auto(proxyOptions, (proxyRes) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As said in the issue's description, we have a server using h2c and performing proxy requests with protocol detection (http2.auto), so it should be fairly easy to have it fully support http/https/http2. However, it didn't feel mandatory to do it in the PR (as it would require more tests to make sure the behavior is fully preserved, when with 2 implementation, if my new implementation has issues, it's less of a problem given it's only used with the --http2 flag)

Comment on lines +18 to +31
const kibanaServerConfig = config.get('servers.kibana');
const kibanaServerUrl = formatUrl(kibanaServerConfig);

const options: AgentOptions = {};
if (kibanaServerConfig.certificateAuthorities) {
options.ca = kibanaServerConfig.certificateAuthorities;
options.rejectUnauthorized = false;
}

const serverArgs = config.get('kbnTestServer.serverArgs', []) as string[];
const http2Enabled = serverArgs.includes('--server.protocol=http2');
if (http2Enabled) {
options.http2 = true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making supertest work with self-signed certs and http2 (note: passing a rejectUnauthorized: false agent to supertest isn't enough... you also need the NODE_TLS_REJECT_UNAUTHORIZED var env. Why? I don't know 🤷)

Comment on lines +16 to +31
const kibanaServerConfig = config.get('servers.kibana');
const kibanaServerUrl = formatUrl(kibanaServerConfig);

const options: AgentOptions = {};
if (kibanaServerConfig.certificateAuthorities) {
options.ca = kibanaServerConfig.certificateAuthorities;
options.rejectUnauthorized = false;
}

const serverArgs = config.get('kbnTestServer.serverArgs', []) as string[];
const http2Enabled = serverArgs.includes('--server.protocol=http2');
if (http2Enabled) {
options.http2 = true;
}

return supertest(kibanaServerUrl, options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL: we have a lot of duplicate definitions for our supertest service depending on the suite category...

Comment on lines 14 to 21
/**
* Enables HTTP2 by adding/changing the appropriate config settings
*
* Important: this must be used on "final" (non-reused) configs, otherwise
* the override from the children configs could remote the overrides
* done in that helper.
*/
export const configureHTTP2 = (config: ConfigType): ConfigType => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do all that is required to enable http2 on a given FTR suite.

I just added the helper to more easily toggle http2 on any suite, but we might want something more integrated if we decide we want to be able to switch http2 on and off on any suite.

Comment on lines +20 to +27
const certificateAuthorities = config.get('servers.kibana.certificateAuthorities');
const httpsAgent: Https.Agent | undefined = certificateAuthorities
? new Https.Agent({
ca: certificateAuthorities,
// required for self-signed certificates used for HTTPS FTR testing
rejectUnauthorized: false,
})
: undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note that running a suite using that service confirmed that axios, even if not supporting http2, properly succeed in making its requests downgraded to http1)

Comment on lines +15 to +18
return configureHTTP2({
...functionalConfig.getAll(),
testFiles: [require.resolve('.')],
};
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows the suite to run with http2 enabled.

@pgayvallet pgayvallet marked this pull request as ready for review May 28, 2024 16:16
@pgayvallet pgayvallet requested review from a team as code owners May 28, 2024 16:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Overall this is looking in a great state 🚢


In some situations, it may be required to enable http2 without a valid h2c configuration.

Q: which situations might this be? Most cited cases I see are for performance enhancements.


I tested locally with and without http2 (using the --http2 flag) and confirmed that Kibana loads as expected. I tested Console (most loved app ;) ) and it was working as expected. Overall no issues with implementation or approach taken.

@@ -52,3 +56,23 @@ const configureHttp1Listener = (

return listener;
};

const configureHttp2Listener = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I understanding correctly that this is "all" that is required for hapi to run an http2 server?

Copy link
Contributor Author

@pgayvallet pgayvallet May 30, 2024

Choose a reason for hiding this comment

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

Yes, thanksfully node's listener interfaces are well done enough so that strictly technically speaking, just providing an http2 listener instead of the expected http or https one to the HAPI server "just works".

There are still a few minor issues caused by the fact that HAPI was never updated to officially support http2, e.g.

{
// emitted whenever a header not supported by http2 is set. it's not actionable for the end user.
// HAPI sets a connection: close header - see https://github.com/hapijs/hapi/issues/3830
name: 'UnsupportedWarning',
messageContains:
'header is not valid, the value will be dropped from the header and will never be in use.',
},

But overall, it works out of the box (which was a great news because the cost/effort wouldn't have been the same if we were forced to fork or switch to another library 🙈)

test/common/configure_http2.ts Outdated Show resolved Hide resolved
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label May 29, 2024
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Test changes LGTM.
I think for the moment, it's fine to have these two configs running with http2 enabled (similar to, as you mentioned, the small set of https tests that we have today). We'll think about if and how we want to extend test coverage longer term.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-http-server-internal 78 79 +1
@kbn/server-http-tools 58 64 +6
total +7

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/server-http-tools 1 0 -1
Unknown metric groups

API count

id before after diff
@kbn/core-http-server-internal 91 92 +1
@kbn/server-http-tools 62 69 +7
total +8

History

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

@pgayvallet pgayvallet merged commit dea26c6 into elastic:main Jun 3, 2024
35 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:http release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants