-
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
Using re2 for Timelion regular expressions #55208
Changes from all commits
3127a9b
49d49a2
7ed2ce2
175bcfc
1d2100a
747fb9f
587d86a
4632317
4351c29
57bfd20
ca7d9f7
df1572f
df750f7
7559524
c1f3de2
00f7fdf
f8410db
d9fa9b8
20c5ee5
fc885de
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 |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
import fs from 'fs'; | ||
import path from 'path'; | ||
import util from 'util'; | ||
import { deleteAll, download, untar } from '../lib'; | ||
|
||
const BASE_URL = `https://storage.googleapis.com/native-modules`; | ||
const DOWNLOAD_DIRECTORY = '.native_modules'; | ||
|
||
const packages = [ | ||
{ | ||
name: 're2', | ||
version: '1.10.5', | ||
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. question If I understand this correctly, then the version number for 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. It's only used to determine the name of the file that we should download. I don't know of a way to verify the version that we download, as it's just a gzipped tar of the 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 was mostly concerned with the two-step process for updating native modules. First, we bump the version in If someone tried to bump the version only via 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. Ah, gotcha. That makes sense, lemme figure out what we can do about that. |
||
destinationPath: 'node_modules/re2/build/Release/', | ||
shas: { | ||
darwin: '066533b592094f91e00412499e44c338ce2466d63c9eaf0dc32be8214bde2099', | ||
linux: '0322cac3c2e106129b650a8eac509f598ed283791d6116984fec4c151b24e574', | ||
windows: '65b5bef7de2352f4787224c2c76a619b6683a868c8d4d71e0fdd500786fc422b', | ||
}, | ||
}, | ||
]; | ||
|
||
async function getInstalledVersion(config, packageName) { | ||
const packageJSONPath = config.resolveFromRepo( | ||
path.join('node_modules', packageName, 'package.json') | ||
); | ||
const buffer = await util.promisify(fs.readFile)(packageJSONPath); | ||
const packageJSON = JSON.parse(buffer); | ||
return packageJSON.version; | ||
} | ||
|
||
async function patchModule(config, log, build, platform, pkg) { | ||
const installedVersion = await getInstalledVersion(config, pkg.name); | ||
if (installedVersion !== pkg.version) { | ||
throw new Error( | ||
`Can't patch ${pkg.name}'s native module, we were expecting version ${pkg.version} and found ${installedVersion}` | ||
); | ||
} | ||
const platformName = platform.getName(); | ||
const archiveName = `${pkg.version}-${platformName}.tar.gz`; | ||
const downloadUrl = `${BASE_URL}/${pkg.name}/${archiveName}`; | ||
const downloadPath = config.resolveFromRepo(DOWNLOAD_DIRECTORY, archiveName); | ||
const extractedPath = build.resolvePathForPlatform(platform, pkg.destinationPath); | ||
log.debug(`Patching ${pkg.name} binaries from ${downloadUrl} to ${extractedPath}`); | ||
|
||
await deleteAll([extractedPath], log); | ||
await download({ | ||
log, | ||
url: downloadUrl, | ||
destination: downloadPath, | ||
sha256: pkg.shas[platformName], | ||
retries: 3, | ||
}); | ||
await untar(downloadPath, extractedPath); | ||
watson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
export const PatchNativeModulesTask = { | ||
description: 'Patching platform-specific native modules', | ||
async run(config, log, build) { | ||
for (const pkg of packages) { | ||
await Promise.all( | ||
config.getTargetPlatforms().map(async platform => { | ||
await patchModule(config, log, build, platform, pkg); | ||
}) | ||
); | ||
} | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,10 @@ export default new Chainable('label', { | |
const config = args.byName; | ||
return alter(args, function(eachSeries) { | ||
if (config.regex) { | ||
eachSeries.label = eachSeries.label.replace(new RegExp(config.regex), config.label); | ||
// not using a standard `import` so that if there's an issue with the re2 native module | ||
// that it doesn't prevent Kibana from starting up and we only have an issue using Timelion labels | ||
const RE2 = require('re2'); | ||
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. 🏅 Good foresight here, I like how this is handled. We will end up leaking whatever error is thrown to the UI. This is probably ok, but wanted to point that out in case you didn't intend for this. 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. We generally display error messages thrown on the server in the client, so I figured doing so here was acceptable as well. It's usually just the stack trace which we consider to include sensitive information. |
||
eachSeries.label = eachSeries.label.replace(new RE2(config.regex), config.label); | ||
} else { | ||
eachSeries.label = config.label; | ||
} | ||
|
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.
question Do we control this URL? I'm having a hard time following elastic/node-re2#pre-build
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.
We do :) https://github.com/elastic/node-re2/tree/pre-build currently builds to a temporary personal AWS bucket because of infra/legal issues, and the the files are copied to our GCP storage bucket, where we pull it from.
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.
Gotcha -- are we going to wait on merging this until those infra issues are resolved, or are we trusting the temporary personal bucket?
The SHA check we have would prevent a build from succeeding if the assets were tampered with though, right?
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.
I was planning on merging before the infra issues are resolved. I'd like to see whether or not https://github.com/uhop/node-re2 is amenable to using a pre-built version of
node-pre-gyp
so we don't have to maintain our fork which does the one-off build.The SHA is generated after the build succeeds.
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.
I should have clarified, The SHA check we have would prevent a Kibana build from succeeding if the assets were tampered with though
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.
Yup, absolutely! We just have to trust whomever/whatever built them and transferred them to that bucket (which was me and Travis CI).