Skip to content

Commit

Permalink
New Platform and Legacy platform servers integration (#39047) (#39263)
Browse files Browse the repository at this point in the history
* New and Legacy platforms share http server instance.

Required to use a common security interceptor for incoming http requests

* generate docs

* remove excessive contract method

* add test for New platform compatibility

* address comments part #1

* log server running only for http server

* fix test. mutate hapi request headers for BWC with legacy

* return 503 on start

* address @eli comments

* address @joshdover comments
  • Loading branch information
mshustov authored Jun 19, 2019
1 parent 986b70c commit 2466399
Show file tree
Hide file tree
Showing 30 changed files with 326 additions and 576 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@

## HttpServiceStart.isListening property

Indicates if http server is listening on a port
Indicates if http server is listening on a given port

<b>Signature:</b>

```typescript
isListening: () => boolean;
isListening: (port: number) => boolean;
```
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ export interface HttpServiceStart

| Property | Type | Description |
| --- | --- | --- |
| [isListening](./kibana-plugin-server.httpservicestart.islistening.md) | <code>() =&gt; boolean</code> | Indicates if http server is listening on a port |
| [isListening](./kibana-plugin-server.httpservicestart.islistening.md) | <code>(port: number) =&gt; boolean</code> | Indicates if http server is listening on a given port |

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,5 @@ export interface InternalCoreStart

| Property | Type | Description |
| --- | --- | --- |
| [http](./kibana-plugin-server.internalcorestart.http.md) | <code>HttpServiceStart</code> | |
| [plugins](./kibana-plugin-server.internalcorestart.plugins.md) | <code>PluginsServiceStart</code> | |

Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export async function runKibanaServer({ procs, config, options }) {
...process.env,
},
cwd: installDir || KIBANA_ROOT,
wait: /Server running/,
wait: /http server running/,
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ describe('Server logging configuration', function () {
'--logging.json', 'false'
]);

watchFileUntil(logPath, /Server running at/, 2 * minute)
watchFileUntil(logPath, /http server running/, 2 * minute)
.then(() => {
// once the server is running, archive the log file and issue SIGHUP
fs.renameSync(logPath, logPathArchived);
Expand Down
25 changes: 0 additions & 25 deletions src/core/server/http/__snapshots__/http_server.test.ts.snap

This file was deleted.

21 changes: 21 additions & 0 deletions src/core/server/http/__snapshots__/http_service.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions src/core/server/http/http_server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ test('listening after started', async () => {
await server.start();

expect(server.isListening()).toBe(true);
expect(loggingServiceMock.collect(logger).info).toMatchInlineSnapshot(`
Array [
Array [
"http server running",
],
]
`);
});

test('200 OK with body', async () => {
Expand Down Expand Up @@ -579,11 +586,10 @@ test('returns server and connection options on start', async () => {
...config,
port: 12345,
};
const { options, server: innerServer } = await server.setup(configWithPort);
const { server: innerServer } = await server.setup(configWithPort);

expect(innerServer).toBeDefined();
expect(innerServer).toBe((server as any).server);
expect(options).toMatchSnapshot();
});

test('registers registerOnPostAuth interceptor several times', async () => {
Expand Down
10 changes: 6 additions & 4 deletions src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { Request, Server, ServerOptions } from 'hapi';
import { Request, Server } from 'hapi';

import { Logger } from '../logging';
import { HttpConfig } from './http_config';
Expand All @@ -37,7 +37,6 @@ import { BasePath } from './base_path_service';

export interface HttpServerSetup {
server: Server;
options: ServerOptions;
registerRouter: (router: Router) => void;
/**
* To define custom authentication and/or authorization mechanism for incoming requests.
Expand Down Expand Up @@ -114,7 +113,6 @@ export class HttpServer {
this.setupBasePathRewrite(config, basePathService);

return {
options: serverOptions,
registerRouter: this.registerRouter.bind(this),
registerOnPreAuth: this.registerOnPreAuth.bind(this),
registerOnPostAuth: this.registerOnPostAuth.bind(this),
Expand Down Expand Up @@ -156,7 +154,8 @@ export class HttpServer {

await this.server.start();
const serverPath = this.config!.rewriteBasePath || this.config!.basePath || '';
this.log.debug(`http server running at ${this.server.info.uri}${serverPath}`);
this.log.info('http server running');
this.log.debug(`http server listening on ${this.server.info.uri}${serverPath}`);
}

public async stop() {
Expand Down Expand Up @@ -231,6 +230,9 @@ export class HttpServer {
authenticate: adoptToHapiAuthFormat(fn, (req, { state, headers }) => {
this.authState.set(req, state);
this.authHeaders.set(req, headers);
// we mutate headers only for the backward compatibility with the legacy platform.
// where some plugin read directly from headers to identify whether a user is authenticated.
Object.assign(req.headers, headers);
}),
}));
this.server.auth.strategy('session', 'login');
Expand Down
3 changes: 1 addition & 2 deletions src/core/server/http/http_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { Server, ServerOptions } from 'hapi';
import { Server } from 'hapi';
import { HttpService } from './http_service';
import { HttpServerSetup } from './http_server';
import { HttpServiceSetup } from './http_service';
Expand All @@ -27,7 +27,6 @@ type ServiceSetupMockType = jest.Mocked<HttpServiceSetup> & {
};
const createSetupContractMock = () => {
const setupContract: ServiceSetupMockType = {
options: ({} as unknown) as ServerOptions,
// we can mock some hapi server method when we need it
server: {} as Server,
registerOnPreAuth: jest.fn(),
Expand Down
102 changes: 91 additions & 11 deletions src/core/server/http/http_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { noop } from 'lodash';
import { BehaviorSubject } from 'rxjs';
import { HttpService, Router } from '.';
import { HttpConfigType, config } from './http_config';
import { httpServerMock } from './http_server.mocks';
import { Config, ConfigService, Env, ObjectToConfigAdapter } from '../config';
import { loggingServiceMock } from '../logging/logging_service.mock';
import { getEnvOptions } from '../config/__mocks__/env';
Expand All @@ -43,6 +44,11 @@ const createConfigService = (value: Partial<HttpConfigType> = {}) => {
configService.setSchema(config.path, config.schema);
return configService;
};
const fakeHapiServer = {
start: noop,
stop: noop,
route: noop,
};

afterEach(() => {
jest.clearAllMocks();
Expand All @@ -56,9 +62,9 @@ test('creates and sets up http server', async () => {

const httpServer = {
isListening: () => false,
setup: jest.fn(),
setup: jest.fn().mockReturnValue({ server: fakeHapiServer }),
start: jest.fn(),
stop: noop,
stop: jest.fn(),
};
mockHttpServer.mockImplementation(() => httpServer);

Expand All @@ -69,11 +75,62 @@ test('creates and sets up http server', async () => {
expect(httpServer.setup).not.toHaveBeenCalled();

await service.setup();
expect(httpServer.setup).toHaveBeenCalledTimes(1);
expect(httpServer.setup).toHaveBeenCalled();
expect(httpServer.start).not.toHaveBeenCalled();

await service.start();
expect(httpServer.start).toHaveBeenCalledTimes(1);
expect(httpServer.start).toHaveBeenCalled();
});

test('spins up notReady server until started if configured with `autoListen:true`', async () => {
const configService = createConfigService();
const httpServer = {
isListening: () => false,
setup: jest.fn(),
start: jest.fn(),
stop: jest.fn(),
};
const notReadyHapiServer = {
start: jest.fn(),
stop: jest.fn(),
route: jest.fn(),
};

mockHttpServer
.mockImplementationOnce(() => httpServer)
.mockImplementationOnce(() => ({
setup: () => ({ server: notReadyHapiServer }),
}));

const service = new HttpService({
configService,
env: new Env('.', getEnvOptions()),
logger,
});

await service.setup();

const mockResponse: any = {
code: jest.fn().mockImplementation(() => mockResponse),
header: jest.fn().mockImplementation(() => mockResponse),
};
const mockResponseToolkit = {
response: jest.fn().mockReturnValue(mockResponse),
};

const [[{ handler }]] = notReadyHapiServer.route.mock.calls;
const response503 = await handler(httpServerMock.createRawRequest(), mockResponseToolkit);
expect(response503).toBe(mockResponse);
expect({
body: mockResponseToolkit.response.mock.calls,
code: mockResponse.code.mock.calls,
header: mockResponse.header.mock.calls,
}).toMatchSnapshot('503 response');

await service.start();

expect(httpServer.start).toBeCalledTimes(1);
expect(notReadyHapiServer.stop).toBeCalledTimes(1);
});

// this is an integration test!
Expand Down Expand Up @@ -121,7 +178,7 @@ test('logs error if already set up', async () => {

const httpServer = {
isListening: () => true,
setup: jest.fn(),
setup: jest.fn().mockReturnValue({ server: fakeHapiServer }),
start: noop,
stop: noop,
};
Expand All @@ -139,7 +196,7 @@ test('stops http server', async () => {

const httpServer = {
isListening: () => false,
setup: noop,
setup: jest.fn().mockReturnValue({ server: fakeHapiServer }),
start: noop,
stop: jest.fn(),
};
Expand All @@ -157,13 +214,39 @@ test('stops http server', async () => {
expect(httpServer.stop).toHaveBeenCalledTimes(1);
});

test('stops not ready server if it is running', async () => {
const configService = createConfigService();
const mockHapiServer = {
start: jest.fn(),
stop: jest.fn(),
route: jest.fn(),
};
const httpServer = {
isListening: () => false,
setup: jest.fn().mockReturnValue({ server: mockHapiServer }),
start: noop,
stop: jest.fn(),
};
mockHttpServer.mockImplementation(() => httpServer);

const service = new HttpService({ configService, env, logger });

await service.setup();

await service.stop();

expect(mockHapiServer.stop).toHaveBeenCalledTimes(1);
});

test('register route handler', async () => {
const configService = createConfigService();

const registerRouterMock = jest.fn();
const httpServer = {
isListening: () => false,
setup: () => ({ registerRouter: registerRouterMock }),
setup: jest
.fn()
.mockReturnValue({ server: fakeHapiServer, registerRouter: registerRouterMock }),
start: noop,
stop: noop,
};
Expand All @@ -181,10 +264,7 @@ test('register route handler', async () => {

test('returns http server contract on setup', async () => {
const configService = createConfigService();
const httpServer = {
server: {},
options: { someOption: true },
};
const httpServer = { server: fakeHapiServer, options: { someOption: true } };

mockHttpServer.mockImplementation(() => ({
isListening: () => false,
Expand Down
Loading

0 comments on commit 2466399

Please sign in to comment.