[http] explicitly create the server listener#183591
Conversation
pgayvallet
added
Feature:http
Team:Core
|
/ci |
| config: IHttpConfig, | ||
| options: GetServerListenerOptions = {} | ||
| ): ServerListener { | ||
| return configureHttp1Listener(config, options); |
There was a problem hiding this comment.
This is where we will properly branch to configure an http2 listener instead.
| }); | ||
|
|
||
| return server; | ||
| export function createServer(serverOptions: ServerOptions) { |
There was a problem hiding this comment.
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.
| // 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, |
There was a problem hiding this comment.
This is the concrete change of configuration introduced by this PR
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We do use it (once), for the redirect server:
| /** | ||
| * 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; |
There was a problem hiding this comment.
(just preparing for the next step by introducing a proper listener type)
|
/ci |
|
/ci |
…nge-listener-config
|
/ci |
|
Pinging @elastic/kibana-core (Team:Core) |
| keepAliveTimeout: config.keepaliveTimeout, | ||
| }); | ||
|
|
||
| listener.setTimeout(config.socketTimeout); |
There was a problem hiding this comment.
We used to define a .on('timeout', handler) too.
Is this no longer needed?
There was a problem hiding this comment.
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!
| createServer, | ||
| IHttpConfig, | ||
| } from '@kbn/server-http-tools'; | ||
| import { getServerOptions, createServer, IHttpConfig } from '@kbn/server-http-tools'; |
There was a problem hiding this comment.
NIT IHttpConfig can be imported as a type.
| import { getServerListener } from './get_listener'; | ||
|
|
||
| const createConfig = ( | ||
| parts: Partial<Omit<IHttpConfig, 'ssl'>> & { ssl?: Partial<IHttpConfig['ssl']> } |
There was a problem hiding this comment.
I've tried to simplify this locally, and TS does not complain when doing:
parts: Partial<IHttpConfig>
There was a problem hiding this comment.
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
|
|
||
| const createConfig = (parts: Partial<IHttpConfig>): IHttpConfig => ({ | ||
| const createConfig = ( | ||
| parts: Partial<Omit<IHttpConfig, 'ssl'>> & { ssl?: Partial<IHttpConfig['ssl']> } |
There was a problem hiding this comment.
gsoldevila
left a comment
gsoldevila
left a comment
There was a problem hiding this comment.
Overall LGTM! Just a few minor doubts / suggestions, and one question around the on('timeout', handler).
…nge-listener-config
…nge-listener-config
…nge-listener-config
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
6 participants
Summary
Related to #7104
Adapted from #183465
For
http2support, 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
httporhttps(tls) listener and passing it when creating the HAPI server instead of just passing thetlsoptions.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.