-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Serve static assets from NP #60490
Serve static assets from NP #60490
Changes from 8 commits
a3cc842
9bb4937
aaafdc6
475a5c3
6aa6e56
fabb201
a2394df
e03576a
05e026c
0dec6ca
048c385
70e9ffc
dc10e99
3d7dcb0
862d230
d0678c9
3021e43
59759d7
753f2c4
38fb7be
9912b9a
30effed
ce9fbb5
4aad487
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import { Agent as HttpsAgent, ServerOptions as TlsOptions } from 'https'; | |
import apm from 'elastic-apm-node'; | ||
import { ByteSizeValue } from '@kbn/config-schema'; | ||
import { Server, Request, ResponseToolkit } from 'hapi'; | ||
import HapiProxy from 'h2o2'; | ||
import { sample } from 'lodash'; | ||
import BrowserslistUserAgent from 'browserslist-useragent'; | ||
import * as Rx from 'rxjs'; | ||
|
@@ -102,7 +103,7 @@ export class BasePathProxyServer { | |
|
||
// Register hapi plugin that adds proxying functionality. It can be configured | ||
// through the route configuration object (see { handler: { proxy: ... } }). | ||
await this.server.register({ plugin: require('h2o2') }); | ||
await this.server.register([HapiProxy]); | ||
|
||
if (this.httpConfig.ssl.enabled) { | ||
const tlsOptions = serverOptions.tls as TlsOptions; | ||
|
@@ -166,7 +167,8 @@ export class BasePathProxyServer { | |
host: this.server.info.host, | ||
passThrough: true, | ||
port: this.devConfig.basePathProxyTargetPort, | ||
protocol: this.server.info.protocol, | ||
// typings mismatch. h2o2 doesn't support "socket" | ||
protocol: this.server.info.protocol as HapiProxy.ProxyHandlerOptions['protocol'], | ||
xforward: true, | ||
}, | ||
}, | ||
|
@@ -195,16 +197,17 @@ export class BasePathProxyServer { | |
agent: this.httpsAgent, | ||
passThrough: true, | ||
xforward: true, | ||
mapUri: (request: Request) => ({ | ||
mshustov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
uri: Url.format({ | ||
hostname: request.server.info.host, | ||
port: this.devConfig.basePathProxyTargetPort, | ||
protocol: request.server.info.protocol, | ||
pathname: `${this.httpConfig.basePath}/${request.params.kbnPath}`, | ||
query: request.query, | ||
mapUri: (request: Request) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed type errors |
||
Promise.resolve({ | ||
uri: Url.format({ | ||
hostname: request.server.info.host, | ||
port: this.devConfig.basePathProxyTargetPort, | ||
protocol: request.server.info.protocol, | ||
pathname: `${this.httpConfig.basePath}/${request.params.kbnPath}`, | ||
query: request.query, | ||
}), | ||
headers: request.headers, | ||
}), | ||
headers: request.headers, | ||
}), | ||
}, | ||
}, | ||
method: '*', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS | |
this.log.info('Plugin initialization disabled.'); | ||
} else { | ||
contracts = await this.pluginsSystem.setupPlugins(deps); | ||
this.registerPluginStaticDirs(deps); | ||
} | ||
|
||
const uiPlugins = this.pluginsSystem.uiPlugins(); | ||
|
@@ -217,6 +218,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS | |
if (plugin.includesUiPlugin) { | ||
this.uiPluginInternalInfo.set(plugin.name, { | ||
publicTargetDir: Path.resolve(plugin.path, 'target/public'), | ||
publicAssetsDir: Path.resolve(plugin.path, 'public/assets'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LP exposes the whole folder https://github.com/elastic/kibana/pull/60490/files#diff-15c0a3d71c405cd45e19b0114aecb64eR76 |
||
}); | ||
} | ||
|
||
|
@@ -256,4 +258,13 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS | |
) | ||
); | ||
} | ||
|
||
private registerPluginStaticDirs(deps: PluginsServiceSetupDeps) { | ||
for (const [pluginName, pluginInfo] of this.uiPluginInternalInfo) { | ||
deps.http.registerStaticDir( | ||
`/plugins/${pluginName}/assets/{path*}`, | ||
pluginInfo.publicAssetsDir | ||
); | ||
Comment on lines
+270
to
+273
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How should we document this feature? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added docs to the migration guide. Let me know if I can improve something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that a temporary measure to allow plugins to move their static assets to NP plugins until #50654 lands? Didn't we want to make all assets exposition explicit? Or is this going to be permanent to allow client-only plugins to expose assets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm on the fence here. On the one hand, we prefer explicitness, but it increases the API surface, makes a folder structure less predictable, enforces server-side plugin usage. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -194,6 +194,10 @@ export interface InternalPluginInfo { | |
* served | ||
*/ | ||
readonly publicTargetDir: string; | ||
/** | ||
* Path to the plugin assets directory. | ||
*/ | ||
readonly publicAssetsDir: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to server build artifacts from the |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"id": "corePluginStaticAssets", | ||
"version": "0.0.1", | ||
"kibanaVersion": "kibana", | ||
"server": false, | ||
"ui": true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"name": "corePluginStaticAssets", | ||
"version": "1.0.0", | ||
"main": "target/test/plugin_functional/plugins/core_plugin_static_assets", | ||
"kibana": { | ||
"version": "kibana", | ||
"templateVersion": "1.0.0" | ||
}, | ||
"license": "Apache-2.0", | ||
"scripts": { | ||
"kbn": "node ../../../../scripts/kbn.js", | ||
"build": "rm -rf './target' && tsc" | ||
}, | ||
"devDependencies": { | ||
"typescript": "3.7.2" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. We sure were doing fun things.