[http] explicitly create the server listener #183591

pgayvallet · 2024-05-16T07:24:08Z

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 · 2024-05-16T07:24:38Z

/ci

pgayvallet

self-review

pgayvallet · 2024-05-16T07:27:11Z

packages/kbn-server-http-tools/src/get_listener.ts

+  config: IHttpConfig,
+  options: GetServerListenerOptions = {}
+): ServerListener {
+  return configureHttp1Listener(config, options);


This is where we will properly branch to configure an http2 listener instead.

pgayvallet · 2024-05-16T07:27:56Z

packages/kbn-server-http-tools/src/create_server.ts

-  });
-
-  return server;
+export function createServer(serverOptions: ServerOptions) {


The root createServer function is even simplified, because the options returned by createServerOptions now contain everything.

Note: I did not delete the function because it still provides a good isolation and separation of concern.

pgayvallet · 2024-05-16T07:29:19Z

packages/kbn-server-http-tools/src/get_server_options.ts

+    // manually configuring the listener
+    listener: getServerListener(config, { configureTLS }),
+    // must set to true when manually passing a TLS listener, false otherwise
+    tls: configureTLS && config.ssl.enabled,


This is the concrete change of configuration introduced by this PR

Probably not in the scope of this PR, but I'm wondering why we have a configureTLS parameter. We don't seem to have any use case where we set a false value for it.

Perhaps I missed something, but I have a feeling that it should either be removed, or surfaced and put together with the rest of IHttpConfig.

We do use it (once), for the redirect server:

kibana/packages/core/http/core-http-server-internal/src/https_redirect_server.ts

Lines 31 to 37 in 0d53e16

// Redirect server is configured in the same way as any other HTTP server

// within the platform with the only exception that it should always be a

// plain HTTP server, so we just ignore `tls` part of options.

this.server = createServer({

...getServerOptions(config, { configureTLS: false }),

port: config.ssl.redirectHttpFromPort,

});

pgayvallet · 2024-05-16T07:29:52Z

packages/kbn-server-http-tools/src/types.ts

+/**
+ * Composite type of all possible kind of Listener types.
+ *
+ * Unfortunately, there's no real common interface between all those concrete classes,
+ * as `net.Server` and `tls.Server` don't list all the APIs we're using (such as event binding)
+ */
+export type ServerListener = HttpServer | HttpsServer;


(just preparing for the next step by introducing a proper listener type)

pgayvallet · 2024-05-16T12:48:52Z

/ci

pgayvallet · 2024-05-16T13:59:24Z

/ci

…nge-listener-config

pgayvallet · 2024-05-16T14:00:07Z

/ci

elasticmachine · 2024-05-17T06:54:38Z

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

gsoldevila · 2024-05-21T08:44:26Z

packages/kbn-server-http-tools/src/get_listener.ts

+        keepAliveTimeout: config.keepaliveTimeout,
+      });
+
+  listener.setTimeout(config.socketTimeout);


We used to define a .on('timeout', handler) too.
Is this no longer needed?

Na, good catch, I totally missed to re-add the

server.listener.on('timeout', (socket) => { socket.destroy(); });

TBH I'm not sure this is really useful (given I assume sockets will be destroyed on timeout anyway), but let's the purpose of the PR is not to test that, so I will add it back. Thanks!

gsoldevila · 2024-05-21T08:50:01Z

packages/kbn-cli-dev-mode/src/integration_tests/base_path_proxy_server.test.ts

-  createServer,
-  IHttpConfig,
-} from '@kbn/server-http-tools';
+import { getServerOptions, createServer, IHttpConfig } from '@kbn/server-http-tools';


NIT IHttpConfig can be imported as a type.

gsoldevila · 2024-05-21T09:01:23Z

packages/kbn-server-http-tools/src/get_listener.test.ts

+import { getServerListener } from './get_listener';
+
+const createConfig = (
+  parts: Partial<Omit<IHttpConfig, 'ssl'>> & { ssl?: Partial<IHttpConfig['ssl']> }


I've tried to simplify this locally, and TS does not complain when doing:

parts: Partial<IHttpConfig>

Pretty sure I had to adapt because partial !== deep partial, and I was using deep partials for config.ssl, but I will check again

EDIT: Nope, you're right. I guess I changed how I manipulate the config creation at some point in the test

gsoldevila · 2024-05-21T09:02:07Z

packages/kbn-server-http-tools/src/get_server_options.test.ts

@@ -20,7 +21,9 @@ jest.mock('fs', () => {
  };
 });

-const createConfig = (parts: Partial<IHttpConfig>): IHttpConfig => ({
+const createConfig = (
+  parts: Partial<Omit<IHttpConfig, 'ssl'>> & { ssl?: Partial<IHttpConfig['ssl']> }


Same as https://github.com/elastic/kibana/pull/183591/files#r1607927873

gsoldevila

Overall LGTM! Just a few minor doubts / suggestions, and one question around the on('timeout', handler).

…nge-listener-config

gsoldevila

LGTM! 🚀

…nge-listener-config

kibana-ci · 2024-05-21T15:11:21Z

💚 Build Succeeded

Buildkite Build
Commit: a045140

Metrics [docs]

✅ unchanged

History

💔 Build #211211 failed 746d04d
💔 Build #211208 failed 746d04d
💚 Build #211171 succeeded d2aba9c
💚 Build #210498 succeeded 77ca04a
💔 Build #210475 failed 870849d
💔 Build #210368 failed 0d53e16

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

[http] explicitly create the server listener

0d53e16

pgayvallet added Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.15.0 labels May 16, 2024

pgayvallet commented May 16, 2024

View reviewed changes

fix health server usages

870849d

fix more tests

38c4ee6

pgayvallet closed this May 16, 2024

pgayvallet reopened this May 16, 2024

Merge remote-tracking branch 'upstream/main' into kbn-7104-prelim-cha…

77ca04a

…nge-listener-config

pgayvallet marked this pull request as ready for review May 17, 2024 06:54

pgayvallet requested review from a team as code owners May 17, 2024 06:54

jbudz approved these changes May 17, 2024

View reviewed changes

gsoldevila reviewed May 21, 2024

View reviewed changes

review comments

b6c5318

pgayvallet requested a review from gsoldevila May 21, 2024 09:56

Merge remote-tracking branch 'upstream/main' into kbn-7104-prelim-cha…

d2aba9c

…nge-listener-config

gsoldevila approved these changes May 21, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-7104-prelim-cha…

746d04d

…nge-listener-config

pgayvallet enabled auto-merge (squash) May 21, 2024 12:04

Merge remote-tracking branch 'upstream/main' into kbn-7104-prelim-cha…

a045140

…nge-listener-config

pgayvallet merged commit db316ad into elastic:main May 21, 2024
22 checks passed

kibanamachine added the backport:skip This commit does not require backporting label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[http] explicitly create the server listener #183591

[http] explicitly create the server listener #183591

pgayvallet commented May 16, 2024 •

edited

pgayvallet commented May 16, 2024

pgayvallet left a comment

pgayvallet May 16, 2024

pgayvallet May 16, 2024

pgayvallet May 16, 2024

gsoldevila May 21, 2024

pgayvallet May 21, 2024 •

edited

pgayvallet May 16, 2024

pgayvallet commented May 16, 2024

pgayvallet commented May 16, 2024

pgayvallet commented May 16, 2024

elasticmachine commented May 17, 2024

gsoldevila May 21, 2024 •

edited

pgayvallet May 21, 2024

gsoldevila May 21, 2024

gsoldevila May 21, 2024

pgayvallet May 21, 2024 •

edited

gsoldevila May 21, 2024

gsoldevila left a comment

gsoldevila left a comment

kibana-ci commented May 21, 2024

	// Redirect server is configured in the same way as any other HTTP server
	// within the platform with the only exception that it should always be a
	// plain HTTP server, so we just ignore `tls` part of options.
	this.server = createServer({
	...getServerOptions(config, { configureTLS: false }),
	port: config.ssl.redirectHttpFromPort,
	});

[http] explicitly create the server listener #183591

[http] explicitly create the server listener #183591

Conversation

pgayvallet commented May 16, 2024 • edited

Summary

pgayvallet commented May 16, 2024

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented May 16, 2024

pgayvallet commented May 16, 2024

pgayvallet commented May 16, 2024

elasticmachine commented May 17, 2024

gsoldevila May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoldevila left a comment

Choose a reason for hiding this comment

gsoldevila left a comment

Choose a reason for hiding this comment

kibana-ci commented May 21, 2024

💚 Build Succeeded

Metrics [docs]

History

pgayvallet commented May 16, 2024 •

edited

pgayvallet May 21, 2024 •

edited

gsoldevila May 21, 2024 •

edited

pgayvallet May 21, 2024 •

edited