[optimize] inject publicPath at request time #14007

spalger · 2017-09-15T01:39:11Z

Webpack bundles are now created using a placeholder for the publicPath rather than embedding the basePath directly into the assets. This means that:

the same build assets can be used for any basePath
we have to replace the placeholder at request time

The logic for serving these "dynamic" assets is completely based on hapijs/Inert, which is what was serving the assets before now. It's not exactly trivial but it does protect against path traversal attacks, produce and cache etags, efficiently use file descriptors, and doesn't require forking or hacking Inert/Hapi.

If we wanted to use a single handler for all files we would also need to add some of the lookup tricks supported by Inert's directory handler does, but I'm not convinced that's better.

epixa · 2017-09-15T13:11:37Z

This is great, but it seems like way too large of a change to drop into 6.0. Let's target 6.1 so it can go through a full QA run.

epixa · 2017-09-15T13:20:04Z

I'm not against this approach as I do think we need a solution to this one way or another, but I can't help but feel like <base> is the right answer here, since it's the prescribed solution to this exact problem in the html spec itself. I'm not entirely sure why the approach was abandoned in #10854. @jbudz had mentioned that it doesn't impact relative paths in CSS files, but that shouldn't matter at all since paths in CSS files are relative to the location of the CSS file itself rather than the html document that loaded it, so this whole class of problem doesn't seem to apply there.

@spalger Do you think <base> is not an appropriate solution here? If so, why?

spalger · 2017-09-15T21:06:18Z

I was also hopeful for <base> but couldn't find any way to get it to work for CSS. It also broke a bunch of relative links, which felt like a very confusing bwc break.

epixa · 2017-09-15T21:14:28Z

To me, <base> seems like a large up front effort with a huge reward: a consistent spec'd behavior baked directly into browsers to solve a problem that may otherwise take 2 dependencies and 600 lines of code to support ourselves...

Again, I'm not against this particular change, but it seems like a stopgap to me. I'd like to better understand the issues around <base> though - do you remember what CSS problems you encountered? The tag shouldn't have any effect on relative paths inside css files, as those paths are relative to the css file itself rather than to the html document that linked to them.

spalger · 2017-09-16T11:04:53Z

Alright, I gave up too early on using <base>, but I'm still concerned about the changes necessary to get links working again. This is what the change set looks like for Kibana: master...spalger:base-tag

The worst breaking change is that links like <a href="#"> now link to <a href="/basepath#"> regardless of the current route, which means everything reloads the page and opens discover... Hopefully there aren't that many users using the #app/... style urls that we used extensively internally, but it also wouldn't surprise me. We might be able to use an href directive to fix some of them, or maybe a global click handler that tries to fix links automatically... or maybe we just want to pull off the bandaid...

epixa · 2017-09-16T13:01:05Z

If we do this, I think pulling off the bandaid is probably a good idea in the long run. We can create a blog post about the change in advance to provide details to plugin authors and whatnot, just like we did with the elasticsearch cluster changes in 5.2.

So the real question is whether we're OK with removing support for relative paths, which is effectively what <base> provides. I think the consistency that <base> provides is worth that limitation, but we should cast a wider net here to get feedback from the whole team.

kimjoar · 2017-09-18T08:00:31Z

++ I like the idea of moving to <base>.

Also agree on pulling off the bandaid. We shouldn't have links to <a href="#"> either way (the UI framework already contains styling for having a button look like a link).

spalger · 2017-09-25T17:39:21Z

Alright, we explored the idea of using <base> in #14045, and while it's certainly possible it also introduces a major breaking change in the way Kibana interprets URLs. Many of us were not comfortable with the impact of that change and many of us also want to fix #11015, which will have a similar impact, so we've tentatively agreed to try doing both changes at the same time in the 7.0 timeframe and plan to use this fix for now.

@kjbekkelund @epixa @jbudz @azasypkin who's excited to review this?!

…uest-time-public-path-replace

kimjoar · 2017-09-26T19:40:03Z

👍 I'll try to get this reviewed tomorrow

kimjoar

LGTM. Added a couple comments that are semi-nitpicky. Tested on both Kibana and xpack and didn't hit any issues.

kimjoar · 2017-09-28T17:14:34Z

src/optimize/bundles_route/file_hash.js

+
+/**
+ *  Get the hash of a file via a file descriptor
+ *  @param  {Fs.FileDescriptor} fd


Add the others params

kimjoar · 2017-09-28T17:46:20Z

src/optimize/bundles_route/bundles_route.js

+ *  @return {undefined}
+ */
+export function createBundlesRoute({ bundlesPath, basePublicPath }) {
+  const cache = new LruCache(100);


Either too generically named or needs a comment, especially as it's not used in this file either. Maybe fileHashCache or something like that(?)

kimjoar · 2017-09-28T17:47:37Z

src/optimize/bundles_route/dynamic_asset_response.js

+ *  @property {Hapi.Request} options.request
+ *  @property {string} options.bundlesPath
+ *  @property {string} options.publicPath
+ *  @property {MapLike} [options.cache]


cache isn't optional, as far as I can tell

kimjoar · 2017-09-28T17:48:48Z

src/optimize/bundles_route/dynamic_asset_response.js

+import { replacePlaceholder } from '../public_path_placeholder';
+
+/**
+ *  Create a Hapi response for the requested path


A bit more explanation maybe? Mention caching etc. Maybe pull in some of the info from your PR comment?

kimjoar · 2017-09-28T17:56:49Z

src/optimize/bundles_route/bundles_route.js

+ *  @param {Object} options
+ *  @property {string} options.bundlesPath
+ *  @property {string} options.basePublicPath
+ *  @return {undefined}


undefined is not correct, it's returning an object

…uest-time-public-path-replace

spalger · 2017-09-28T19:33:41Z

Thanks for the feedback @kjbekkelund, should all be addressed. Hopefully @epixa @jbudz or @azasypkin have a chance to look into this soon 😄

azasypkin

LGTM, haven't noticed anything suspicious while testing so far.

azasypkin · 2017-10-02T14:02:41Z

src/optimize/bundles_route/bundles_route.js

+  // will store the 100 most recently used hashes.
+  const fileHashCache = new LruCache(100);
+
+  if (typeof bundlesPath !== 'string' || !isAbsolute(bundlesPath)) {


question: is typeof bundlesPath !== 'string check needed just to have more user friendly error message (in contrast to what node will throw by itself here and here)? I just find it useful that node uses require('util').inspect inside argument asserts :) No need to change anything, just curious.

Yeah, I can include the bundlesPath value in the error message, but I figure the only time this will be triggered is when the user is monkeying around with the config file, so the more expressive error message will be more useful than seeing their value (and makes the error message longer, maybe wrap, etc.)

azasypkin · 2017-10-02T14:11:08Z

src/optimize/bundles_route/bundles_route.js

+
+  return {
+    method: 'GET',
+    path: `/bundles/{path*}`,


optional nit: quoting string with good old ' should be enough here.

azasypkin · 2017-10-02T14:23:27Z

src/optimize/bundles_route/bundles_route.js

+        onPreHandler: {
+          method(request, reply) {
+            const ext = extname(request.params.path);
+            if (ext !== '.js' && ext !== '.css') {


question: Should .js.map files be treated as .js files?

Hmm... Good question... I'm tempted to say that since the placeholder only shows up in the webpack loader that processing sourcemap files isn't necessary. None of the source that is actually on disk should be using the placeholder, and if it happens to be the source maps shouldn't really be impacted outside of the line with the replacement.

azasypkin · 2017-10-02T15:31:21Z

src/optimize/bundles_route/dynamic_asset_response.js

+import { open, fstat, createReadStream, close } from 'fs';
+
+import Boom from 'boom';
+import { fromNode as fcb } from 'bluebird';


question: since we started to use RxJS, maybe you can use Rx.Observable.bindNodeCallback + toPromise instead of Bluebird (IIRC we'd like to eventually get rid of this lib), what do you think?

I don't think we should start using RxJS outside of where it's especially helpful, yet. It's still unclear what direction we are going to go when it comes to which observable library we use

azasypkin · 2017-10-02T16:30:42Z

src/optimize/bundles_route/__tests__/bundles_route.js

+          bundlesPath: '/bundles',
+          basePublicPath: 'a/'
+        });
+      }).to.throwError(/start and not end with a \//);


nit: could you please add case for the happy path too, as in the test above?

azasypkin · 2017-10-02T16:37:55Z

src/optimize/bundles_route/__tests__/bundles_route.js

+}
+
+function assertBufferMatch(a, b) {
+  if (a.length !== b.length || a.indexOf(b) !== 0) {


question: why not just expect(a).to.eql(b) instead of assertBufferMatch? It seems to be supported...

Huh, I guess I didn't check

Makes sense I suppose, they're basically just arrays

azasypkin · 2017-10-02T16:43:45Z

src/optimize/bundles_route/__tests__/bundles_route.js

+
+      expect(response.statusCode).to.be(200);
+      const image = readFileSync(resolve(outputFixture, 'image.png'));
+      expect(response.headers).to.have.property('content-length', image.length);


optional nit: our code is responsible for setting correct content-type now, maybe it makes sense to verify it here too.

azasypkin · 2017-10-02T18:13:31Z

src/utils/streams/replace_stream.js

@@ -0,0 +1,46 @@
+import { Transform } from 'stream';


nit: would be great to have some unit tests for this file :)

azasypkin · 2017-10-02T18:21:39Z

src/utils/streams/replace_stream.js

+          buffer = buffer.slice(index + toReplace.length);
+        }
+
+        if (buffer.length > replacement.length) {


nit: could you please add comment explaining what this if case is for?

Nice, this was actually a bug: 8ca1c33

…Replace, not replacement

* [optimize] inject publicPath at request time * [optimize/getFileHash] finish doc block * [optimize/bundlesRoute] correct return value doc type * [optimize/bundleRoute] use more descriptive name for file hash cache * [optimize/dynamicAssetResponse] add more details to doc * [utils/createReplaceStream] trim the buffer based on the length of toReplace, not replacement * [utils/createReplaceStream] add inline docs * [utils/createReplaceStream] write unit tests * [optimize/bundleRoute] expect supports buffers * [optimize/bundleRoute/basePublicPath/tests] add happy path * [optimize/bundlesRoute/tests] verify content-type header * [optimize/bundlesRoute] use ' (cherry picked from commit 1ea82fa)

* [optimize] inject publicPath at request time * [optimize/getFileHash] finish doc block * [optimize/bundlesRoute] correct return value doc type * [optimize/bundleRoute] use more descriptive name for file hash cache * [optimize/dynamicAssetResponse] add more details to doc * [utils/createReplaceStream] trim the buffer based on the length of toReplace, not replacement * [utils/createReplaceStream] add inline docs * [utils/createReplaceStream] write unit tests * [optimize/bundleRoute] expect supports buffers * [optimize/bundleRoute/basePublicPath/tests] add happy path * [optimize/bundlesRoute/tests] verify content-type header * [optimize/bundlesRoute] use '

spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review v6.0.0 v6.1.0 v7.0.0 labels Sep 15, 2017

spalger requested review from epixa, jbudz and rhoboat September 15, 2017 01:39

[optimize] inject publicPath at request time

9c31dbe

spalger removed v6.0.0 v7.0.0 labels Sep 20, 2017

spalger changed the base branch from master to 6.x September 20, 2017 00:46

spalger force-pushed the implement/request-time-public-path-replace branch from 6c65d67 to 1a16412 Compare September 20, 2017 00:47

elastic deleted a comment from karmi Sep 20, 2017

spalger changed the base branch from 6.x to master September 20, 2017 00:49

spalger force-pushed the implement/request-time-public-path-replace branch from 1a16412 to 9c31dbe Compare September 20, 2017 00:49

spalger mentioned this pull request Sep 25, 2017

[optimize] Use <base> tag for predicatable relative paths #14045

Closed

spalger added the v7.0.0 label Sep 25, 2017

spalger requested review from kimjoar and azasypkin and removed request for rhoboat September 25, 2017 17:39

Merge branch 'master' of github.com:elastic/kibana into implement/req…

c48ae07

…uest-time-public-path-replace

kimjoar approved these changes Sep 28, 2017

View reviewed changes

spalger added 5 commits September 28, 2017 11:56

Merge branch 'master' of github.com:elastic/kibana into implement/req…

c0cfd57

…uest-time-public-path-replace

[optimize/getFileHash] finish doc block

d6244ec

[optimize/bundlesRoute] correct return value doc type

b8943e8

[optimize/bundleRoute] use more descriptive name for file hash cache

b9b8617

[optimize/dynamicAssetResponse] add more details to doc

e0be3b8

azasypkin approved these changes Oct 2, 2017

View reviewed changes

spalger added 7 commits October 2, 2017 17:17

[utils/createReplaceStream] trim the buffer based on the length of to…

8ca1c33

…Replace, not replacement

[utils/createReplaceStream] add inline docs

2162dde

[utils/createReplaceStream] write unit tests

5044600

[optimize/bundleRoute] expect supports buffers

6d0cf8b

[optimize/bundleRoute/basePublicPath/tests] add happy path

71e9bfc

[optimize/bundlesRoute/tests] verify content-type header

4fc8706

[optimize/bundlesRoute] use '

dafaee9

spalger merged commit 1ea82fa into elastic:master Oct 3, 2017

chrisronline mentioned this pull request Oct 4, 2017

Unable to start functional test server #14295

Closed

spalger deleted the implement/request-time-public-path-replace branch October 18, 2019 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[optimize] inject publicPath at request time #14007

[optimize] inject publicPath at request time #14007

spalger commented Sep 15, 2017

epixa commented Sep 15, 2017

epixa commented Sep 15, 2017

spalger commented Sep 15, 2017

epixa commented Sep 15, 2017

spalger commented Sep 16, 2017

epixa commented Sep 16, 2017

kimjoar commented Sep 18, 2017

spalger commented Sep 25, 2017 •

edited

Loading

kimjoar commented Sep 26, 2017

kimjoar left a comment

kimjoar Sep 28, 2017

kimjoar Sep 28, 2017

kimjoar Sep 28, 2017

kimjoar Sep 28, 2017

kimjoar Sep 28, 2017

spalger commented Sep 28, 2017

azasypkin left a comment

azasypkin Oct 2, 2017

spalger Oct 2, 2017 •

edited

Loading

azasypkin Oct 2, 2017

azasypkin Oct 2, 2017

spalger Oct 2, 2017 •

edited

Loading

azasypkin Oct 2, 2017

spalger Oct 2, 2017 •

edited

Loading

azasypkin Oct 2, 2017

azasypkin Oct 2, 2017

spalger Oct 3, 2017

spalger Oct 3, 2017

azasypkin Oct 2, 2017

azasypkin Oct 2, 2017

azasypkin Oct 2, 2017

spalger Oct 3, 2017

[optimize] inject publicPath at request time #14007

[optimize] inject publicPath at request time #14007

Conversation

spalger commented Sep 15, 2017

epixa commented Sep 15, 2017

epixa commented Sep 15, 2017

spalger commented Sep 15, 2017

epixa commented Sep 15, 2017

spalger commented Sep 16, 2017

epixa commented Sep 16, 2017

kimjoar commented Sep 18, 2017

spalger commented Sep 25, 2017 • edited Loading

kimjoar commented Sep 26, 2017

kimjoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Sep 28, 2017

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Sep 25, 2017 •

edited

Loading

spalger Oct 2, 2017 •

edited

Loading

spalger Oct 2, 2017 •

edited

Loading

spalger Oct 2, 2017 •

edited

Loading