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

Using re2 for Timelion regular expressions #55208

Merged
merged 20 commits into from
Mar 12, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/.es
.DS_Store
.node_binaries
.native_modules
node_modules
!/src/dev/npm/integration_tests/__fixtures__/fixture1/node_modules
!/src/dev/notice/__fixtures__/node_modules
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@
"pug": "^2.0.4",
"querystring-browser": "1.0.4",
"raw-loader": "3.1.0",
"re2": "1.10.5",
"react": "^16.12.0",
"react-color": "^2.13.8",
"react-dom": "^16.12.0",
Expand Down
2 changes: 2 additions & 0 deletions src/dev/build/build_distributables.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
ExtractNodeBuildsTask,
InstallDependenciesTask,
OptimizeBuildTask,
PatchNativeModulesTask,
RemovePackageJsonDepsTask,
RemoveWorkspacesTask,
TranspileBabelTask,
Expand Down Expand Up @@ -129,6 +130,7 @@ export async function buildDistributables(options) {
* directories and perform platform-specific steps
*/
await run(CreateArchivesSourcesTask);
await run(PatchNativeModulesTask);
await run(CleanExtraBinScriptsTask);
await run(CleanExtraBrowsersTask);
await run(CleanNodeBuildsTask);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import chalk from 'chalk';
import { createHash } from 'crypto';
import Axios from 'axios';

import { mkdirp } from '../../lib';
import { mkdirp } from './fs';

function tryUnlink(path) {
try {
Expand Down
1 change: 1 addition & 0 deletions src/dev/build/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,6 @@ export {
compress,
isFileAccessible,
} from './fs';
export { download } from './download';
export { scanDelete } from './scan_delete';
export { scanCopy } from './scan_copy';
1 change: 1 addition & 0 deletions src/dev/build/tasks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export * from './nodejs_modules';
export * from './notice_file_task';
export * from './optimize_task';
export * from './os_packages';
export * from './patch_native_modules_task';
export * from './transpile_babel_task';
export * from './transpile_scss_task';
export * from './verify_env_task';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import expect from '@kbn/expect';

import * as NodeShasumsNS from '../node_shasums';
import * as NodeDownloadInfoNS from '../node_download_info';
import * as DownloadNS from '../download';
import * as DownloadNS from '../../../lib/download'; // sinon can't stub '../../../lib' properly
import { DownloadNodeBuildsTask } from '../download_node_builds_task';

describe('src/dev/build/tasks/nodejs/download_node_builds_task', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/dev/build/tasks/nodejs/download_node_builds_task.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { download } from './download';
import { download } from '../../lib';
import { getNodeShasums } from './node_shasums';
import { getNodeDownloadInfo } from './node_download_info';

Expand Down
67 changes: 67 additions & 0 deletions src/dev/build/tasks/patch_native_modules_task.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* 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 { deleteAll, download, untar } from '../lib';

const BASE_URL = `https://storage.googleapis.com/native-modules`;
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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?

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 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.

Copy link
Member

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

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 should have clarified, The SHA check we have would prevent a Kibana build from succeeding if the assets were tampered with though

Yup, absolutely! We just have to trust whomever/whatever built them and transferred them to that bucket (which was me and Travis CI).

const DOWNLOAD_DIRECTORY = '.native_modules';

const packages = [
{
name: 're2',
version: '1.10.5',
Copy link
Member

Choose a reason for hiding this comment

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

question If I understand this correctly, then the version number for re2 that we specify in package.json doesn't do a whole lot. Should we consider validating that the version we download/patch matches the requested version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 re2.node binary. The binary format is different per platform, so if we were downloading on Linux we couldn't do anything meaningful with the binaries for macOS or Windows. The checksums were meant to be our indirect version protection.

Copy link
Member

Choose a reason for hiding this comment

The 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 package.json, but then we have to know to also update patch_native_modules_task.js, which is a step that's unique to native modules.

If someone tried to bump the version only via package.json, they might have a hard time figuring out why their changes aren't taking effect. I think we should be able to compare the version specified in package.json against the version specified here, and throw an error if there is a mismatch

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 patchModule(config, log, build, platform, pkg) {
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);
}

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);
})
);
}
},
};
3 changes: 2 additions & 1 deletion src/plugins/timelion/server/series_functions/label.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import RE2 from 're2';
import { i18n } from '@kbn/i18n';
import alter from '../lib/alter.js';
import Chainable from '../lib/classes/chainable';
Expand Down Expand Up @@ -51,7 +52,7 @@ 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);
eachSeries.label = eachSeries.label.replace(new RE2(config.regex), config.label);
} else {
eachSeries.label = config.label;
}
Expand Down
12 changes: 12 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -20993,6 +20993,11 @@ nan@^2.9.2:
resolved "https://registry.yarnpkg.com/nan/-/nan-2.10.0.tgz#96d0cd610ebd58d4b4de9cc0c6828cda99c7548f"
integrity sha512-bAdJv7fBLhWC+/Bls0Oza+mvTaNQtP+1RyhhhvD95pgUJz6XM5IzgmxOkItJ9tkoCiplvAnXI1tNmmUD/eScyA==

nan@^2.14.0:
version "2.14.0"
resolved "https://registry.yarnpkg.com/nan/-/nan-2.14.0.tgz#7818f722027b2459a86f0295d434d1fc2336c52c"
integrity sha512-INOFj37C7k3AfaNTtX8RhsTw7qRy7eLET14cROi9+5HAVbbHuIWUHEauBv5qT4Av2tWasiTY1Jw6puUNqRJXQg==

nano-css@^5.2.1:
version "5.2.1"
resolved "https://registry.yarnpkg.com/nano-css/-/nano-css-5.2.1.tgz#73b8470fa40b028a134d3393ae36bbb34b9fa332"
Expand Down Expand Up @@ -23988,6 +23993,13 @@ re-resizable@^6.1.1:
dependencies:
fast-memoize "^2.5.1"

re2@1.10.5:
version "1.10.5"
resolved "https://registry.yarnpkg.com/re2/-/re2-1.10.5.tgz#b3730438121c6bf59d459aff3471177eef513445"
integrity sha512-ssO3AD8/YJzuQUgEasS8PxA8n1yg8JB2VNSJhCebuuHLwaGiufhtFGUypS2bONrCPDbjwlMy7OZD9LkcrQMr1g==
dependencies:
nan "^2.14.0"

react-ace@^5.5.0:
version "5.10.0"
resolved "https://registry.yarnpkg.com/react-ace/-/react-ace-5.10.0.tgz#e328b37ac52759f700be5afdb86ada2f5ec84c5e"
Expand Down