[build] Check SHA sum of downloaded node package

[ppisljar]

checks SHA checksum for downloaded node packages and throws error if cheksum doesnt match (after up to 3 retries).
--skip-node-download option can be passed to grunt task to skip download of node builds in case they were downloaded before. SHA verification still happens and throws an error in case it fails.

Fixes #7136
Fixes #7482

[epixa]

The reason why we put downloadNodeBuilds:finish further down the build task is to allow other build tasks to occur while the binaries are downloading. I'm not opposed to blocking entirely on the downloadNodeBuilds task if that's necessary, but if it is, we should roll it all into a single task.

[ppisljar]

i think that download usually takes less than 1 minute and the rest of the build process takes around 10 minutes. so it doesn't feel right to wait for 10 minutes just to find out that my node binaries are corrupt. however we could put it a bit further down the line. in my opinion joining the tasks into one would be the cleanest solution (as download is usually fast).

let me know

[epixa]

Joining them works for me.

[Bargs]

Haven't figured out exactly why yet, but sha checks are failing for me when I run a build.

[Bargs]

Shouldn't we download the checksums first? This looks like a race condition waiting to happen. And I think ideally we'd pass them in as an argument rather than using a mutable variable in the closure.

[Bargs]

Ah. So I think because the SHA check happens in the download function, it gets skipped the second time you run a build, even if it failed the first time (line 20 skips everything if the directory already exists). We should probably re-check the SHA every time a build is run, regardless of whether the binary was just downloaded, or pre-existing.

[Bargs]

You should be able to get rid of a lot of the complexity by using wreck.get, like this:

    const resp = await fromNode(cb => {
      wreck.get(shaSumsUri, function (err, resp, payload) {
        console.log('in get');
        if (err) {
          return cb(err);
        }

        if (resp.statusCode !== 200) {
          return cb(new Error(`${shaSumsUri} failed with a ${resp.statusCode} response`));
        }

        cb(null, payload);
      });
    });

    resp.toString('utf8').split('\n').forEach(line => {
      let cells = line.split('  ');
      shaSums[cells[1]] = cells[0];
    });

[spalger]

jenkins, test it

[epixa]

1.0.1 is the latest.

[ppisljar]

@Bargs my assumption was that if download was successful we don't need to check shasums the next time. shasum is only checked after download, if its correct archive is extracted and next time we don't need to check shasums. if download failed archive wont be extracted ... so it should be downloaded and checked again. why do you think it would be necesary to recheck shasums every time ?

[Bargs]

@ppisljar this line skips everything if the platform directory exists. Running the build twice, it will always succeed the second time because the bad downloads still exist in that directory and their SHA isn't rechecked. So I think we should either re-check them or re-download them every time.

[ppisljar]

i was under wrong assumption that folder will only exist in case download was successful (content extracted). i added code which will remove folder in case download failed. this way it should be redownloaded next time, but wont be redownloading in case download was successful (so we don't unnecessarily slow down the build + we can build while offline once packages have been downloaded)

[epixa]

Honestly, I think it's safer to check the shasum each time regardless of download. The build process is how we create production quality release candidates, so the process should be resilient to outside factors. It shouldn't be possible for me to manually add a file to the node binary directory and have the build process bundle it up.

In other words, assumptions like "well, there's a file where it is suppose to be so I assume we're good" are not cautious enough when it comes to creating official releases.

[ppisljar]

ok, so i try to do this now:

• check if files are already there, if not start download
• once files are downloaded (or have been there already) do a shasum check
• if shasum check is succesfull extract archives (everytime)
• if shasum check has failed i delete the folder and throw error (next time archieve will be redownloaded)

does this sound ok ?

[epixa]

Works for me

[epixa]

@ppisljar Just a few more things:

1. Can you rename --skip-download to --skip-node-download?
2. There's still a let that should be a const: https://github.com/elastic/kibana/pull/7746/files#diff-021efbed7c1f8b71b2ed5c3e72084880R19
3. When I do skip downloading, it should log a message that says as much rather than displaying nothing at all.
4. If I elect to skip downloading, then the only thing that should happen is the sha verification of all downloads. If any sha is incorrect, the task should simply error rather than trying to reconcile it by redownloading it. The task doesn't need to be self healing - you either have all valid node binaries or you must not skip downloading.

[epixa]

Assuming that build goes green, LGTM

Can you update the PR description with more information about this change before merging? Specifically, describe the new skip-node-download argument and that sha verification happens regardless of whether you skip downloads or not.

[ppisljar]

jenkins, test this