Skip to content

Commit

Permalink
[Http] Replace buildNr with buildSha in static asset paths (elast…
Browse files Browse the repository at this point in the history
…ic#175898)

## Summary

Follow up of [first CDN
PR](elastic#169408). Primary focus is
replacing our build nr with SHA that allows cache busting and maintains
anti-collision properties.

## How to test

Start Kibana as usual navigating around the app with the network tab
open in your browser of choice. Keep an eye out for any asset loading
errors. It's tricky to test every possible asset since there are many
permutations, but generally navigating around Kibana should work exactly
as it did before regarding loading bundles and assets.

## Notes
* did a high-level audit of usages of `buildNum` in `packages`, `src`
and `x-pack` adding comments where appropriate.
* In non-distributable builds (like dev) static asset paths will be
prefixed with `XXXXXXXXXXXX` instead of Node's `Number.MAX_SAFE_INTEGER`
* Added some validation to ensure the CDN url is of the expected form:
nothing trailing the pathname

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### Risk Matrix

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| We break some first or third party dependencies on existing asset
routes | Med | High | Attempting to mitgate by serving static assets
from both old and new paths where paths have updated to include the
build SHA. Additioanlly: it is very bad practice to rely on the values
of the static paths, but someone might be |
| Cache-busting is more aggressive | High | Low | Unlikely to be a big
problem, but we are not scoping more static assets to a SHA and so every
new Kibana release will require clients to, for example, download fonts
again. |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
2 people authored and fkanout committed Feb 7, 2024
1 parent 8f3b978 commit 450e776
Show file tree
Hide file tree
Showing 63 changed files with 685 additions and 243 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const PAGE_VARS_KEYS = [

// Deployment-specific keys
'version', // x4, split to version_major, version_minor, version_patch for easier filtering
'buildNum', // May be useful for Serverless
'buildNum', // May be useful for Serverless, TODO: replace with buildHash
'cloudId',
'deploymentId',
'projectId', // projectId and deploymentId are mutually exclusive. They shouldn't be sent in the same offering.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ import { httpServiceMock } from '@kbn/core-http-server-mocks';
import type { InternalPluginInfo, UiPlugins } from '@kbn/core-plugins-base-server-internal';
import { registerBundleRoutes } from './register_bundle_routes';
import { FileHashCache } from './file_hash_cache';
import { BasePath, StaticAssets } from '@kbn/core-http-server-internal';

const createPackageInfo = (parts: Partial<PackageInfo> = {}): PackageInfo => ({
buildNum: 42,
buildSha: 'sha',
buildSha: 'shasha',
buildShaShort: 'sha',
dist: true,
branch: 'master',
version: '8.0.0',
Expand All @@ -41,9 +43,12 @@ const createUiPlugins = (...ids: string[]): UiPlugins => ({

describe('registerBundleRoutes', () => {
let router: ReturnType<typeof httpServiceMock.createRouter>;
let staticAssets: StaticAssets;

beforeEach(() => {
router = httpServiceMock.createRouter();
const basePath = httpServiceMock.createBasePath('/server-base-path') as unknown as BasePath;
staticAssets = new StaticAssets({ basePath, cdnConfig: {} as any, shaDigest: 'sha' });
});

afterEach(() => {
Expand All @@ -53,7 +58,7 @@ describe('registerBundleRoutes', () => {
it('registers core and shared-dep bundles', () => {
registerBundleRoutes({
router,
serverBasePath: '/server-base-path',
staticAssets,
packageInfo: createPackageInfo(),
uiPlugins: createUiPlugins(),
});
Expand All @@ -64,39 +69,39 @@ describe('registerBundleRoutes', () => {
fileHashCache: expect.any(FileHashCache),
isDist: true,
bundlesPath: 'uiSharedDepsSrcDistDir',
publicPath: '/server-base-path/42/bundles/kbn-ui-shared-deps-src/',
routePath: '/42/bundles/kbn-ui-shared-deps-src/',
publicPath: '/server-base-path/sha/bundles/kbn-ui-shared-deps-src/',
routePath: '/sha/bundles/kbn-ui-shared-deps-src/',
});

expect(registerRouteForBundleMock).toHaveBeenCalledWith(router, {
fileHashCache: expect.any(FileHashCache),
isDist: true,
bundlesPath: 'uiSharedDepsNpmDistDir',
publicPath: '/server-base-path/42/bundles/kbn-ui-shared-deps-npm/',
routePath: '/42/bundles/kbn-ui-shared-deps-npm/',
publicPath: '/server-base-path/sha/bundles/kbn-ui-shared-deps-npm/',
routePath: '/sha/bundles/kbn-ui-shared-deps-npm/',
});

expect(registerRouteForBundleMock).toHaveBeenCalledWith(router, {
fileHashCache: expect.any(FileHashCache),
isDist: true,
bundlesPath: expect.stringMatching(/\/@kbn\/core\/target\/public$/),
publicPath: '/server-base-path/42/bundles/core/',
routePath: '/42/bundles/core/',
publicPath: '/server-base-path/sha/bundles/core/',
routePath: '/sha/bundles/core/',
});

expect(registerRouteForBundleMock).toHaveBeenCalledWith(router, {
fileHashCache: expect.any(FileHashCache),
isDist: true,
bundlesPath: 'kbnMonacoBundleDir',
publicPath: '/server-base-path/42/bundles/kbn-monaco/',
routePath: '/42/bundles/kbn-monaco/',
publicPath: '/server-base-path/sha/bundles/kbn-monaco/',
routePath: '/sha/bundles/kbn-monaco/',
});
});

it('registers plugin bundles', () => {
registerBundleRoutes({
router,
serverBasePath: '/server-base-path',
staticAssets,
packageInfo: createPackageInfo(),
uiPlugins: createUiPlugins('plugin-a', 'plugin-b'),
});
Expand All @@ -107,16 +112,16 @@ describe('registerBundleRoutes', () => {
fileHashCache: expect.any(FileHashCache),
isDist: true,
bundlesPath: '/plugins/plugin-a/public-target-dir',
publicPath: '/server-base-path/42/bundles/plugin/plugin-a/8.0.0/',
routePath: '/42/bundles/plugin/plugin-a/8.0.0/',
publicPath: '/server-base-path/sha/bundles/plugin/plugin-a/8.0.0/',
routePath: '/sha/bundles/plugin/plugin-a/8.0.0/',
});

expect(registerRouteForBundleMock).toHaveBeenCalledWith(router, {
fileHashCache: expect.any(FileHashCache),
isDist: true,
bundlesPath: '/plugins/plugin-b/public-target-dir',
publicPath: '/server-base-path/42/bundles/plugin/plugin-b/8.0.0/',
routePath: '/42/bundles/plugin/plugin-b/8.0.0/',
publicPath: '/server-base-path/sha/bundles/plugin/plugin-b/8.0.0/',
routePath: '/sha/bundles/plugin/plugin-b/8.0.0/',
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { distDir as UiSharedDepsSrcDistDir } from '@kbn/ui-shared-deps-src';
import * as KbnMonaco from '@kbn/monaco/server';
import type { IRouter } from '@kbn/core-http-server';
import type { UiPlugins } from '@kbn/core-plugins-base-server-internal';
import { InternalStaticAssets } from '@kbn/core-http-server-internal';
import { FileHashCache } from './file_hash_cache';
import { registerRouteForBundle } from './bundles_route';

Expand All @@ -28,56 +29,61 @@ import { registerRouteForBundle } from './bundles_route';
*/
export function registerBundleRoutes({
router,
serverBasePath,
uiPlugins,
packageInfo,
staticAssets,
}: {
router: IRouter;
serverBasePath: string;
uiPlugins: UiPlugins;
packageInfo: PackageInfo;
staticAssets: InternalStaticAssets;
}) {
const { dist: isDist, buildNum } = packageInfo;
const { dist: isDist } = packageInfo;
// rather than calculate the fileHash on every request, we
// provide a cache object to `resolveDynamicAssetResponse()` that
// will store the most recently used hashes.
const fileHashCache = new FileHashCache();

const sharedNpmDepsPath = '/bundles/kbn-ui-shared-deps-npm/';
registerRouteForBundle(router, {
publicPath: `${serverBasePath}/${buildNum}/bundles/kbn-ui-shared-deps-npm/`,
routePath: `/${buildNum}/bundles/kbn-ui-shared-deps-npm/`,
publicPath: staticAssets.prependPublicUrl(sharedNpmDepsPath) + '/',
routePath: staticAssets.prependServerPath(sharedNpmDepsPath) + '/',
bundlesPath: UiSharedDepsNpm.distDir,
fileHashCache,
isDist,
});
const sharedDepsPath = '/bundles/kbn-ui-shared-deps-src/';
registerRouteForBundle(router, {
publicPath: `${serverBasePath}/${buildNum}/bundles/kbn-ui-shared-deps-src/`,
routePath: `/${buildNum}/bundles/kbn-ui-shared-deps-src/`,
publicPath: staticAssets.prependPublicUrl(sharedDepsPath) + '/',
routePath: staticAssets.prependServerPath(sharedDepsPath) + '/',
bundlesPath: UiSharedDepsSrcDistDir,
fileHashCache,
isDist,
});
const coreBundlePath = '/bundles/core/';
registerRouteForBundle(router, {
publicPath: `${serverBasePath}/${buildNum}/bundles/core/`,
routePath: `/${buildNum}/bundles/core/`,
publicPath: staticAssets.prependPublicUrl(coreBundlePath) + '/',
routePath: staticAssets.prependServerPath(coreBundlePath) + '/',
bundlesPath: isDist
? fromRoot('node_modules/@kbn/core/target/public')
: fromRoot('src/core/target/public'),
fileHashCache,
isDist,
});
const monacoEditorPath = '/bundles/kbn-monaco/';
registerRouteForBundle(router, {
publicPath: `${serverBasePath}/${buildNum}/bundles/kbn-monaco/`,
routePath: `/${buildNum}/bundles/kbn-monaco/`,
publicPath: staticAssets.prependPublicUrl(monacoEditorPath) + '/',
routePath: staticAssets.prependServerPath(monacoEditorPath) + '/',
bundlesPath: KbnMonaco.bundleDir,
fileHashCache,
isDist,
});

[...uiPlugins.internal.entries()].forEach(([id, { publicTargetDir, version }]) => {
const pluginBundlesPath = `/bundles/plugin/${id}/${version}/`;
registerRouteForBundle(router, {
publicPath: `${serverBasePath}/${buildNum}/bundles/plugin/${id}/${version}/`,
routePath: `/${buildNum}/bundles/plugin/${id}/${version}/`,
publicPath: staticAssets.prependPublicUrl(pluginBundlesPath) + '/',
routePath: staticAssets.prependServerPath(pluginBundlesPath) + '/',
bundlesPath: publicTargetDir,
fileHashCache,
isDist,
Expand Down
22 changes: 19 additions & 3 deletions packages/core/apps/core-apps-server-internal/src/core_app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { httpResourcesMock } from '@kbn/core-http-resources-server-mocks';
import { PluginType } from '@kbn/core-base-common';
import type { RequestHandlerContext } from '@kbn/core-http-request-handler-context-server';
import { coreInternalLifecycleMock } from '@kbn/core-lifecycle-server-mocks';
import { CoreAppsService } from './core_app';
import { of } from 'rxjs';
import { CoreAppsService } from './core_app';

const emptyPlugins = (): UiPlugins => ({
internal: new Map(),
Expand Down Expand Up @@ -146,7 +146,7 @@ describe('CoreApp', () => {
uiPlugins: prebootUIPlugins,
router: expect.any(Object),
packageInfo: coreContext.env.packageInfo,
serverBasePath: internalCorePreboot.http.basePath.serverBasePath,
staticAssets: expect.any(Object),
});
});

Expand Down Expand Up @@ -245,7 +245,23 @@ describe('CoreApp', () => {
uiPlugins,
router: expect.any(Object),
packageInfo: coreContext.env.packageInfo,
serverBasePath: internalCoreSetup.http.basePath.serverBasePath,
staticAssets: expect.any(Object),
});
});

it('registers SHA-scoped and non-SHA-scoped UI bundle routes', async () => {
const uiPlugins = emptyPlugins();
internalCoreSetup.http.staticAssets.prependServerPath.mockReturnValue('/some-path');
await coreApp.setup(internalCoreSetup, uiPlugins);

expect(internalCoreSetup.http.registerStaticDir).toHaveBeenCalledTimes(2);
expect(internalCoreSetup.http.registerStaticDir).toHaveBeenCalledWith(
'/some-path',
expect.any(String)
);
expect(internalCoreSetup.http.registerStaticDir).toHaveBeenCalledWith(
'/ui/{path*}',
expect.any(String)
);
});
});
23 changes: 17 additions & 6 deletions packages/core/apps/core-apps-server-internal/src/core_app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
import type { UiPlugins } from '@kbn/core-plugins-base-server-internal';
import type { HttpResources, HttpResourcesServiceToolkit } from '@kbn/core-http-resources-server';
import type { InternalCorePreboot, InternalCoreSetup } from '@kbn/core-lifecycle-server-internal';
import type { InternalStaticAssets } from '@kbn/core-http-server-internal';
import { firstValueFrom, map, type Observable } from 'rxjs';
import { CoreAppConfig, type CoreAppConfigType, CoreAppPath } from './core_app_config';
import { registerBundleRoutes } from './bundle_routes';
Expand All @@ -33,6 +34,7 @@ interface CommonRoutesParams {
httpResources: HttpResources;
basePath: IBasePath;
uiPlugins: UiPlugins;
staticAssets: InternalStaticAssets;
onResourceNotFound: (
req: KibanaRequest,
res: HttpResourcesServiceToolkit & KibanaResponseFactory
Expand Down Expand Up @@ -77,10 +79,11 @@ export class CoreAppsService {
this.registerCommonDefaultRoutes({
basePath: corePreboot.http.basePath,
httpResources: corePreboot.httpResources.createRegistrar(router),
staticAssets: corePreboot.http.staticAssets,
router,
uiPlugins,
onResourceNotFound: async (req, res) =>
// THe API consumers might call various Kibana APIs (e.g. `/api/status`) when Kibana is still at the preboot
// The API consumers might call various Kibana APIs (e.g. `/api/status`) when Kibana is still at the preboot
// stage, and the main HTTP server that registers API handlers isn't up yet. At this stage we don't know if
// the API endpoint exists or not, and hence cannot reply with `404`. We also should not reply with completely
// unexpected response (`200 text/html` for the Core app). The only suitable option is to reply with `503`
Expand Down Expand Up @@ -125,6 +128,7 @@ export class CoreAppsService {
this.registerCommonDefaultRoutes({
basePath: coreSetup.http.basePath,
httpResources: resources,
staticAssets: coreSetup.http.staticAssets,
router,
uiPlugins,
onResourceNotFound: async (req, res) => res.notFound(),
Expand Down Expand Up @@ -210,6 +214,7 @@ export class CoreAppsService {
private registerCommonDefaultRoutes({
router,
basePath,
staticAssets,
uiPlugins,
onResourceNotFound,
httpResources,
Expand Down Expand Up @@ -259,17 +264,23 @@ export class CoreAppsService {
registerBundleRoutes({
router,
uiPlugins,
staticAssets,
packageInfo: this.env.packageInfo,
serverBasePath: basePath.serverBasePath,
});
}

// After the package is built and bootstrap extracts files to bazel-bin,
// assets are exposed at the root of the package and in the package's node_modules dir
private registerStaticDirs(core: InternalCoreSetup | InternalCorePreboot) {
core.http.registerStaticDir(
'/ui/{path*}',
fromRoot('node_modules/@kbn/core-apps-server-internal/assets')
);
/**
* Serve UI from sha-scoped and not-sha-scoped paths to allow time for plugin code to migrate
* Eventually we only want to serve from the sha scoped path
*/
[core.http.staticAssets.prependServerPath('/ui/{path*}'), '/ui/{path*}'].forEach((path) => {
core.http.registerStaticDir(
path,
fromRoot('node_modules/@kbn/core-apps-server-internal/assets')
);
});
}
}
1 change: 1 addition & 0 deletions packages/core/apps/core-apps-server-internal/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"@kbn/core-lifecycle-server-mocks",
"@kbn/core-ui-settings-server",
"@kbn/monaco",
"@kbn/core-http-server-internal",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function createCoreContext({ production = false }: { production?: boolean } = {}
branch: 'branch',
buildNum: 100,
buildSha: 'buildSha',
buildShaShort: 'buildShaShort',
dist: false,
buildDate: new Date('2023-05-15T23:12:09.000Z'),
buildFlavor: 'traditional',
Expand Down
1 change: 1 addition & 0 deletions packages/core/http/core-http-server-internal/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type {
InternalHttpServiceStart,
} from './src/types';
export { BasePath } from './src/base_path_service';
export { type InternalStaticAssets, StaticAssets } from './src/static_assets';

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { CdnConfig } from './cdn';
import { CdnConfig } from './cdn_config';

describe('CdnConfig', () => {
it.each([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ export interface Input {
}

export class CdnConfig {
private url: undefined | URL;
private readonly url: undefined | URL;
constructor(url?: string) {
if (url) {
this.url = new URL(url); // This will throw for invalid URLs
this.url = new URL(url); // This will throw for invalid URLs, although should be validated before reaching this point
}
}

public get host(): undefined | string {
return this.url?.host ?? undefined;
return this.url?.host;
}

public get baseHref(): undefined | string {
Expand Down
Loading

0 comments on commit 450e776

Please sign in to comment.