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

[http] Do not do client version check on serverless as we do for onprem #159101

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/serverless.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,6 @@ telemetry.allowChangingOptInStatus: false
server.securityResponseHeaders.strictTransportSecurity: max-age=31536000; includeSubDomains
# Disable embedding for serverless MVP
server.securityResponseHeaders.disableEmbedding: true

server.versioned.handlerResolution: newest # default to newest routes
server.versioned.strictClientVersionCheck: false # do not enforce client version check
jloleysens marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 10 additions & 4 deletions packages/core/http/core-http-router-server-internal/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,11 @@ function validOptions(
interface RouterOptions {
/** Whether we are running in development */
isDev?: boolean;
/** Whether we are running in a serverless */
isServerless?: boolean;
/**
* Which route resolution algo to use
* @default 'oldest'
*/
versionedRouteResolution?: 'newest' | 'oldest';
}

/**
Expand All @@ -143,7 +146,10 @@ export class Router<Context extends RequestHandlerContextBase = RequestHandlerCo
public readonly routerPath: string,
private readonly log: Logger,
private readonly enhanceWithContext: ContextEnhancer<any, any, any, any, any>,
private readonly options: RouterOptions = { isDev: false, isServerless: false }
private readonly options: RouterOptions = {
isDev: false,
versionedRouteResolution: 'oldest',
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I know it was done to avoid having to change all the constructor calls, but I wonder how safe having this options be an optional parameter is. I don't think we're using the Router in that many places, so maybe making options mandatory now could make sense, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!

) {
const buildMethod =
<Method extends RouteMethod>(method: Method) =>
Expand Down Expand Up @@ -220,7 +226,7 @@ export class Router<Context extends RequestHandlerContextBase = RequestHandlerCo
this.versionedRouter = CoreVersionedRouter.from({
router: this,
isDev: this.options.isDev,
defaultHandlerResolutionStrategy: this.options.isServerless ? 'newest' : 'oldest',
defaultHandlerResolutionStrategy: this.options.versionedRouteResolution,
});
}
return this.versionedRouter;
Expand Down
8 changes: 6 additions & 2 deletions packages/core/http/core-http-server-internal/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ export type {
} from './src/types';
export { BasePath } from './src/base_path_service';

export { cspConfig, CspConfig } from './src/csp';
export { cspConfig, CspConfig, type CspConfigType } from './src/csp';

export { externalUrlConfig, ExternalUrlConfig } from './src/external_url';
export {
externalUrlConfig,
ExternalUrlConfig,
type ExternalUrlConfigType,
} from './src/external_url';

export { createCookieSessionStorageFactory } from './src/cookie_session_storage';

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

10 changes: 10 additions & 0 deletions packages/core/http/core-http-server-internal/src/http_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,14 @@ const configSchema = schema.object(
}
),
restrictInternalApis: schema.boolean({ defaultValue: false }), // allow access to internal routes by default to prevent breaking changes in current offerings
versioned: schema.object({
// Which handler resolution algo to use: "newest" or "oldest"
handlerResolution: schema.oneOf([schema.literal('newest'), schema.literal('oldest')], {
defaultValue: 'oldest',
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bring this more in line with other ways of configuring Kibana for serverless

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Handler resolution or version resolution? Is the "oldest handler" really a valid term?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change to versionResolution

// Whether we enforce version checks on client requests
strictClientVersionCheck: schema.boolean({ defaultValue: true }),
}),
},
{
validate: (rawConfig) => {
Expand Down Expand Up @@ -239,6 +247,7 @@ export class HttpConfig implements IHttpConfig {
public externalUrl: IExternalUrlConfig;
public xsrf: { disableProtection: boolean; allowlist: string[] };
public requestId: { allowFromAnyIp: boolean; ipAllowlist: string[] };
public versioned: { handlerResolution: 'newest' | 'oldest'; strictClientVersionCheck: boolean };
public shutdownTimeout: Duration;
public restrictInternalApis: boolean;

Expand Down Expand Up @@ -286,6 +295,7 @@ export class HttpConfig implements IHttpConfig {

this.restrictInternalApis = rawHttpConfig.restrictInternalApis;
this.eluMonitor = rawHttpConfig.eluMonitor;
this.versioned = rawHttpConfig.versioned;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import { executionContextServiceMock } from '@kbn/core-execution-context-server-mocks';
import { contextServiceMock } from '@kbn/core-http-context-server-mocks';
import { Router } from '@kbn/core-http-router-server-internal';
jest.mock('@kbn/core-http-router-server-internal');
import { HttpService } from './http_service';
import { HttpConfigType, config } from './http_config';
import { cspConfig } from './csp';
Expand Down Expand Up @@ -480,3 +481,43 @@ test('does not start http server if configured with `autoListen:false`', async (
expect(httpServer.start).not.toHaveBeenCalled();
await service.stop();
});

test('passes versioned config to router', async () => {
const configService = createConfigService({
versioned: {
handlerResolution: 'newest',
strictClientVersionCheck: false,
},
});

const httpServer = {
isListening: () => false,
setup: jest.fn().mockReturnValue({ server: fakeHapiServer, registerRouter: jest.fn() }),
start: jest.fn(),
stop: jest.fn(),
};
const prebootHttpServer = {
isListening: () => false,
setup: jest.fn().mockReturnValue({ server: fakeHapiServer, registerStaticDir: jest.fn() }),
start: jest.fn(),
stop: jest.fn(),
};
mockHttpServer.mockImplementationOnce(() => prebootHttpServer);
mockHttpServer.mockImplementationOnce(() => httpServer);

const service = new HttpService({ coreId, configService, env, logger });
await service.preboot(prebootDeps);
const { createRouter } = await service.setup(setupDeps);
await service.stop();

createRouter('/foo');

expect(Router).toHaveBeenCalledTimes(1);
expect(Router).toHaveBeenNthCalledWith(
1,
'/foo',
expect.any(Object), // logger
expect.any(Function), // context enhancer
expect.objectContaining({ isDev: true, versionedRouteResolution: 'newest' })
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
InternalHttpServiceSetup,
InternalHttpServiceStart,
} from './types';
import { registerCoreHandlers } from './lifecycle_handlers';
import { registerCoreHandlers } from './register_lifecycle_handlers';
import { ExternalUrlConfigType, externalUrlConfig, ExternalUrlConfig } from './external_url';

export interface PrebootDeps {
Expand Down Expand Up @@ -129,7 +129,7 @@ export class HttpService
path,
this.log,
prebootServerRequestHandlerContext.createHandler.bind(null, this.coreContext.coreId),
{ isDev: this.env.mode.dev, isServerless: this.env.cliArgs.serverless }
{ isDev: this.env.mode.dev, versionedRouteResolution: config.versioned.handlerResolution }
);

registerCallback(router);
Expand Down Expand Up @@ -175,7 +175,7 @@ export class HttpService
const enhanceHandler = this.requestHandlerContext!.createHandler.bind(null, pluginId);
const router = new Router<Context>(path, this.log, enhanceHandler, {
isDev: this.env.mode.dev,
isServerless: this.env.cliArgs.serverless,
versionedRouteResolution: config.versioned.handlerResolution,
});
registerRouter(router);
return router;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,24 @@ import type {
OnPreRoutingToolkit,
OnPostAuthHandler,
} from '@kbn/core-http-server';
import { createTestEnv } from '@kbn/config-mocks';
import { mockRouter } from '@kbn/core-http-router-server-mocks';

jest.mock('./lifecycle_handlers', () => {
const actual = jest.requireActual('./lifecycle_handlers');
return {
...actual,
createVersionCheckPostAuthHandler: jest.fn(actual.createVersionCheckPostAuthHandler),
};
});
import {
createCustomHeadersPreResponseHandler,
createRestrictInternalRoutesPostAuthHandler,
createVersionCheckPostAuthHandler,
createXsrfPostAuthHandler,
} from './lifecycle_handlers';
import { registerCoreHandlers } from './register_lifecycle_handlers';

import { HttpConfig } from './http_config';

type ToolkitMock = jest.Mocked<OnPreResponseToolkit & OnPostAuthToolkit & OnPreRoutingToolkit>;
Expand Down Expand Up @@ -55,6 +66,10 @@ const forgeRequest = ({
});
};

afterEach(() => {
jest.clearAllMocks();
});

describe('xsrf post-auth handler', () => {
let toolkit: ToolkitMock;
let responseFactory: ReturnType<typeof mockRouter.createResponseFactory>;
Expand Down Expand Up @@ -424,3 +439,31 @@ describe('customHeaders pre-response handler', () => {
});
});
});

describe('lifecycle handler registration', () => {
it('will not register client version checking if disabled via config', () => {
const registrarMock = {
registerAuth: jest.fn(),
registerOnPostAuth: jest.fn(),
registerOnPreAuth: jest.fn(),
jloleysens marked this conversation as resolved.
Show resolved Hide resolved
registerOnPreResponse: jest.fn(),
registerOnPreRouting: jest.fn(),
};

const config = {
csp: { header: '' },
xsrf: {},
versioned: {
handlerResolution: 'newest',
strictClientVersionCheck: false,
},
} as unknown as HttpConfig;

registerCoreHandlers(registrarMock, config, createTestEnv());
expect(createVersionCheckPostAuthHandler).toHaveBeenCalledTimes(0);

config.versioned.strictClientVersionCheck = true;
registerCoreHandlers(registrarMock, config, createTestEnv());
expect(createVersionCheckPostAuthHandler).toHaveBeenCalledTimes(1);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@
* Side Public License, v 1.
*/

import { Env } from '@kbn/config';
import type { OnPostAuthHandler, OnPreResponseHandler } from '@kbn/core-http-server';
import { isSafeMethod } from '@kbn/core-http-router-server-internal';
import { X_ELASTIC_INTERNAL_ORIGIN_REQUEST } from '@kbn/core-http-common/src/constants';
import { HttpConfig } from './http_config';
import { LifecycleRegistrar } from './http_server';

const VERSION_HEADER = 'kbn-version';
const XSRF_HEADER = 'kbn-xsrf';
Expand Down Expand Up @@ -100,18 +98,3 @@ export const createCustomHeadersPreResponseHandler = (config: HttpConfig): OnPre
return toolkit.next({ headers: additionalHeaders });
};
};

export const registerCoreHandlers = (
registrar: LifecycleRegistrar,
config: HttpConfig,
env: Env
) => {
// add headers based on config
registrar.registerOnPreResponse(createCustomHeadersPreResponseHandler(config));
// add extra request checks stuff
registrar.registerOnPostAuth(createXsrfPostAuthHandler(config));
// add check on version
registrar.registerOnPostAuth(createVersionCheckPostAuthHandler(env.packageInfo.version));
// add check on header if the route is internal
registrar.registerOnPostAuth(createRestrictInternalRoutesPostAuthHandler(config)); // strictly speaking, we should have access to route.options.access from the request on postAuth
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import type { Env } from '@kbn/config';
import type { HttpConfig } from './http_config';
import type { LifecycleRegistrar } from './http_server';
import {
createCustomHeadersPreResponseHandler,
createRestrictInternalRoutesPostAuthHandler,
createVersionCheckPostAuthHandler,
createXsrfPostAuthHandler,
} from './lifecycle_handlers';

export const registerCoreHandlers = (
registrar: LifecycleRegistrar,
config: HttpConfig,
env: Env
) => {
// add headers based on config
registrar.registerOnPreResponse(createCustomHeadersPreResponseHandler(config));
// add extra request checks stuff
registrar.registerOnPostAuth(createXsrfPostAuthHandler(config));
if (config.versioned.strictClientVersionCheck !== false) {
// add check on version
registrar.registerOnPostAuth(createVersionCheckPostAuthHandler(env.packageInfo.version));
}
Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wonder if we still want to warn via some custom header in the response.

This would help when troubleshooting HARs, or even, potentially using that on the browser to warn the user that they should refresh the page.

WDYT?

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 like that idea! As you suggest, this is one way to drive the "update available" UX we've spoken about... Currently it will only detect a new version when we ship a new stack version --- this will not be sufficient for more continuous updates. I think updating the Kibana version to contain a git hash or build nr or something would be needed...

I'd like to spend a bit more time thinking about how we want to approach that specifically. Do you think it is still worth adding a header like this now? If so, do you think we should just reuse the existing kbn-version header and set it to update-available or out-of-sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #159127

Copy link
Contributor

@clintandrewhall clintandrewhall Jul 10, 2023

Choose a reason for hiding this comment

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

IMO, we should just include the version in the header all the time, and add metadata to our logging-- not just error logging-- with the client version and server version. That metadata will be invaluable to not only knowing if a mismatch is contributing to an error, but also if an error has spiked on a particular version of client + server, and how many clients are "lagging" the upgrade curve.

The version mismatch will drive the UX, for certain, but I would love to have requests and errors tagged all the time. The mismatch will be one data point in an investigation... an important one, but not the only one.

// add check on header if the route is internal
registrar.registerOnPostAuth(createRestrictInternalRoutesPostAuthHandler(config)); // strictly speaking, we should have access to route.options.access from the request on postAuth
};
2 changes: 1 addition & 1 deletion packages/core/http/core-http-server-mocks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ export type {
InternalHttpServiceSetupMock,
InternalHttpServiceStartMock,
} from './src/http_service.mock';
export { createCoreContext, createHttpServer } from './src/test_utils';
export { createCoreContext, createHttpServer, createConfigService } from './src/test_utils';
24 changes: 22 additions & 2 deletions packages/core/http/core-http-server-mocks/src/test_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,27 @@ import { Env } from '@kbn/config';
import { getEnvOptions, configServiceMock } from '@kbn/config-mocks';
import type { CoreContext } from '@kbn/core-base-server-internal';
import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import { HttpService } from '@kbn/core-http-server-internal';
import {
type HttpConfigType,
type ExternalUrlConfigType,
type CspConfigType,
HttpService,
} from '@kbn/core-http-server-internal';

const coreId = Symbol('core');
const env = Env.createDefault(REPO_ROOT, getEnvOptions());

const logger = loggingSystemMock.create();

const createConfigService = () => {
export const createConfigService = ({
server,
externalUrl,
csp,
}: Partial<{
server: Partial<HttpConfigType>;
externalUrl: Partial<ExternalUrlConfigType>;
csp: Partial<CspConfigType>;
}> = {}) => {
Comment on lines +29 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 the stub config definition is way better that way

const configService = configServiceMock.create();
configService.atPath.mockImplementation((path) => {
if (path === 'server') {
Expand Down Expand Up @@ -51,18 +64,25 @@ const createConfigService = () => {
keepaliveTimeout: 120_000,
socketTimeout: 120_000,
restrictInternalApis: false,
versioned: {
handlerResolution: 'oldest',
strictClientVersionCheck: true,
},
...server,
} as any);
}
if (path === 'externalUrl') {
return new BehaviorSubject({
policy: [],
...externalUrl,
} as any);
}
if (path === 'csp') {
return new BehaviorSubject({
strict: false,
disableEmbedding: false,
warnLegacyBrowsers: true,
...csp,
});
}
throw new Error(`Unexpected config path: ${path}`);
Expand Down
Loading