Skip to content

Commit

Permalink
feat(aio): use shorter URLs for previews
Browse files Browse the repository at this point in the history 
Use the 7 first characters of the 40-chars long SHAs for shorter/cleaner URLs.
The collision probability is extremely low (since all SHAs are further
"namespaced" under the corresponding PR). In case of a collision, the second PR
will not be deployed, in order to avoid overwriting the original build.

(This is a design decision to keep the implementation simple. It can be changed
later if necessary.)
  • Loading branch information
@gkalpak @matsko
gkalpak authored and matsko committed Jun 29, 2017
1 parent 3c4eef9 commit 4268c82
Show file tree
Hide file tree
Showing 9 changed files with 160 additions and 49 deletions.
2 changes: 1 addition & 1 deletion aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ server {

# Serve PR-preview requests
server {
server_name "~^pr(?<pr>[1-9][0-9]*)-(?<sha>[0-9a-f]{40})\.";
server_name "~^pr(?<pr>[1-9][0-9]*)-(?<sha>[0-9a-f]{7,40})\.";

listen {{$AIO_NGINX_PORT_HTTPS}} ssl http2;
listen [::]:{{$AIO_NGINX_PORT_HTTPS}} ssl http2;
Expand Down
1 change: 1 addition & 0 deletions aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
// Constants
export const HIDDEN_DIR_PREFIX = 'hidden--';
export const SHORT_SHA_LEN = 7;
5 changes: 4 additions & 1 deletion aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {EventEmitter} from 'events';
import * as fs from 'fs';
import * as path from 'path';
import * as shell from 'shelljs';
import {HIDDEN_DIR_PREFIX} from '../common/constants';
import {HIDDEN_DIR_PREFIX, SHORT_SHA_LEN} from '../common/constants';
import {assertNotMissingOrEmpty} from '../common/utils';
import {ChangedPrVisibilityEvent, CreatedBuildEvent} from './build-events';
import {UploadError} from './upload-error';
Expand Down Expand Up @@ -46,6 +46,9 @@ export class BuildCreator extends EventEmitter {
}

public create(pr: string, sha: string, archivePath: string, isPublic: boolean): Promise<void> {
// Use only part of the SHA for more readable URLs.
sha = sha.substr(0, SHORT_SHA_LEN);

const {oldPrDir: otherVisPrDir, newPrDir: prDir} = this.getCandidatePrDirs(pr, isPublic);
const shaDir = path.join(prDir, sha);
let dirToRemoveOnError: string;
Expand Down
33 changes: 22 additions & 11 deletions aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/helper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as fs from 'fs';
import * as http from 'http';
import * as path from 'path';
import * as shell from 'shelljs';
import {HIDDEN_DIR_PREFIX} from '../common/constants';
import {HIDDEN_DIR_PREFIX, SHORT_SHA_LEN} from '../common/constants';
import {getEnvVar} from '../common/utils';

// Constans
Expand Down Expand Up @@ -51,8 +51,9 @@ class Helper {
}

// Methods - Public
public buildExists(pr: string, sha = '', isPublic = true): boolean {
const dir = path.join(this.getPrDir(pr, isPublic), sha);
public buildExists(pr: string, sha = '', isPublic = true, legacy = false): boolean {
const prDir = this.getPrDir(pr, isPublic);
const dir = !sha ? prDir : this.getShaDir(prDir, sha, legacy);
return fs.existsSync(dir);
}

Expand All @@ -68,7 +69,7 @@ class Helper {
}

public createDummyArchive(pr: string, sha: string, archivePath: string): CleanUpFn {
const inputDir = path.join(this.buildsDir, 'uploaded', pr, sha);
const inputDir = this.getShaDir(this.getPrDir(`uploaded/${pr}`, true), sha);
const cmd1 = `tar --create --gzip --directory "${inputDir}" --file "${archivePath}" .`;
const cmd2 = `chown ${this.wwwUser} ${archivePath}`;

Expand All @@ -80,9 +81,9 @@ class Helper {
return this.createCleanUpFn(() => shell.rm('-rf', archivePath));
}

public createDummyBuild(pr: string, sha: string, isPublic = true, force = false): CleanUpFn {
public createDummyBuild(pr: string, sha: string, isPublic = true, force = false, legacy = false): CleanUpFn {
const prDir = this.getPrDir(pr, isPublic);
const shaDir = path.join(prDir, sha);
const shaDir = this.getShaDir(prDir, sha, legacy);
const idxPath = path.join(shaDir, 'index.html');
const barPath = path.join(shaDir, 'foo', 'bar.js');

Expand All @@ -108,9 +109,17 @@ class Helper {
return path.join(this.buildsDir, prDirName);
}

public readBuildFile(pr: string, sha: string, relFilePath: string, isPublic = true): string {
const prDir = this.getPrDir(pr, isPublic);
const absFilePath = path.join(prDir, sha, relFilePath);
public getShaDir(prDir: string, sha: string, legacy = false): string {
return path.join(prDir, legacy ? sha : this.getShordSha(sha));
}

public getShordSha(sha: string): string {
return sha.substr(0, SHORT_SHA_LEN);
}

public readBuildFile(pr: string, sha: string, relFilePath: string, isPublic = true, legacy = false): string {
const shaDir = this.getShaDir(this.getPrDir(pr, isPublic), sha, legacy);
const absFilePath = path.join(shaDir, relFilePath);
return fs.readFileSync(absFilePath, 'utf8');
}

Expand Down Expand Up @@ -156,8 +165,10 @@ class Helper {
};
}

public writeBuildFile(pr: string, sha: string, relFilePath: string, content: string, isPublic = true): CleanUpFn {
const absFilePath = path.join(this.getPrDir(pr, isPublic), sha, relFilePath);
public writeBuildFile(pr: string, sha: string, relFilePath: string, content: string, isPublic = true,
legacy = false): CleanUpFn {
const shaDir = this.getShaDir(this.getPrDir(pr, isPublic), sha, legacy);
const absFilePath = path.join(shaDir, relFilePath);
return this.writeFile(absFilePath, {content}, true);
}

Expand Down
99 changes: 73 additions & 26 deletions aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/nginx.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ describe(`nginx`, () => {
const pr = '9';
const sha9 = '9'.repeat(40);
const sha0 = '0'.repeat(40);
const shortSha9 = h.getShordSha(sha9);
const shortSha0 = h.getShordSha(sha0);


describe(`pr<pr>-<sha>.${host}/*`, () => {
Expand All @@ -50,9 +52,23 @@ describe(`nginx`, () => {


it('should return /index.html', done => {
const origin = `${scheme}://pr${pr}-${shortSha9}.${host}`;
const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /index\\.html$`);

Promise.all([
h.runCmd(`curl -iL ${origin}/index.html`).then(h.verifyResponse(200, bodyRegex)),
h.runCmd(`curl -iL ${origin}/`).then(h.verifyResponse(200, bodyRegex)),
h.runCmd(`curl -iL ${origin}`).then(h.verifyResponse(200, bodyRegex)),
]).then(done);
});


it('should return /index.html (for legacy builds)', done => {
const origin = `${scheme}://pr${pr}-${sha9}.${host}`;
const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /index\\.html$`);

h.createDummyBuild(pr, sha9, true, false, true);

Promise.all([
h.runCmd(`curl -iL ${origin}/index.html`).then(h.verifyResponse(200, bodyRegex)),
h.runCmd(`curl -iL ${origin}/`).then(h.verifyResponse(200, bodyRegex)),
Expand All @@ -64,66 +80,79 @@ describe(`nginx`, () => {
it('should return /foo/bar.js', done => {
const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /foo/bar\\.js$`);

h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/bar.js`).
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo/bar.js`).
then(h.verifyResponse(200, bodyRegex)).
then(done);
});


it('should return /foo/bar.js (for legacy builds)', done => {
const origin = `${scheme}://pr${pr}-${sha9}.${host}`;
const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /foo/bar\\.js$`);

h.createDummyBuild(pr, sha9, true, false, true);

h.runCmd(`curl -iL ${origin}/foo/bar.js`).
then(h.verifyResponse(200, bodyRegex)).
then(done);
});


it('should respond with 403 for directories', done => {
Promise.all([
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/`).then(h.verifyResponse(403)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo`).then(h.verifyResponse(403)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo/`).then(h.verifyResponse(403)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo`).then(h.verifyResponse(403)),
]).then(done);
});


it('should respond with 404 for unknown paths to files', done => {
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/baz.css`).
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo/baz.css`).
then(h.verifyResponse(404)).
then(done);
});


it('should rewrite to \'index.html\' for unknown paths that don\'t look like files', done => {
const origin = `${scheme}://pr${pr}-${shortSha9}.${host}`;
const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /index\\.html$`);

Promise.all([
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/baz`).then(h.verifyResponse(200, bodyRegex)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/baz/`).then(h.verifyResponse(200, bodyRegex)),
h.runCmd(`curl -iL ${origin}/foo/baz`).then(h.verifyResponse(200, bodyRegex)),
h.runCmd(`curl -iL ${origin}/foo/baz/`).then(h.verifyResponse(200, bodyRegex)),
]).then(done);
});


it('should respond with 404 for unknown PRs/SHAs', done => {
const otherPr = 54321;
const otherSha = '8'.repeat(40);
const otherShortSha = h.getShordSha('8'.repeat(40));

Promise.all([
h.runCmd(`curl -iL ${scheme}://pr${pr}9-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${otherPr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}9.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${otherSha}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}9-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${otherPr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}9.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${otherShortSha}.${host}`).then(h.verifyResponse(404)),
]).then(done);
});


it('should respond with 404 if the subdomain format is wrong', done => {
Promise.all([
h.runCmd(`curl -iL ${scheme}://xpr${pr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://prx${pr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://xx${pr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://p${pr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://r${pr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://${pr}-${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}_${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://xpr${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://prx${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://xx${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://p${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://r${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}_${shortSha9}.${host}`).then(h.verifyResponse(404)),
]).then(done);
});


it('should reject PRs with leading zeros', done => {
h.runCmd(`curl -iL ${scheme}://pr0${pr}-${sha9}.${host}`).
h.runCmd(`curl -iL ${scheme}://pr0${pr}-${shortSha9}.${host}`).
then(h.verifyResponse(404)).
then(done);
});
Expand All @@ -134,9 +163,9 @@ describe(`nginx`, () => {
const bodyRegex0 = new RegExp(`^PR: ${pr} | SHA: ${sha0} | File: /index\\.html$`);

Promise.all([
h.runCmd(`curl -iL ${scheme}://pr${pr}-0${sha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}`).then(h.verifyResponse(200, bodyRegex9)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha0}.${host}`).then(h.verifyResponse(200, bodyRegex0)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-0${shortSha9}.${host}`).then(h.verifyResponse(404)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}`).then(h.verifyResponse(200, bodyRegex9)),
h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha0}.${host}`).then(h.verifyResponse(200, bodyRegex0)),
]).then(done);
});

Expand All @@ -145,13 +174,31 @@ describe(`nginx`, () => {

describe('(for hidden builds)', () => {

beforeEach(() => h.createDummyBuild(pr, sha9, false));
it('should respond with 404 for any file or directory', done => {
const origin = `${scheme}://pr${pr}-${shortSha9}.${host}`;
const assert404 = h.verifyResponse(404);

h.createDummyBuild(pr, sha9, false);
expect(h.buildExists(pr, sha9, false)).toBe(true);

it('should respond with 404 for any file or directory', done => {
Promise.all([
h.runCmd(`curl -iL ${origin}/index.html`).then(assert404),
h.runCmd(`curl -iL ${origin}/`).then(assert404),
h.runCmd(`curl -iL ${origin}`).then(assert404),
h.runCmd(`curl -iL ${origin}/foo/bar.js`).then(assert404),
h.runCmd(`curl -iL ${origin}/foo/`).then(assert404),
h.runCmd(`curl -iL ${origin}/foo`).then(assert404),
]).then(done);
});


it('should respond with 404 for any file or directory (for legacy builds)', done => {
const origin = `${scheme}://pr${pr}-${sha9}.${host}`;
const assert404 = h.verifyResponse(404);

h.createDummyBuild(pr, sha9, false, false, true);
expect(h.buildExists(pr, sha9, false, true)).toBe(true);

Promise.all([
h.runCmd(`curl -iL ${origin}/index.html`).then(assert404),
h.runCmd(`curl -iL ${origin}/`).then(assert404),
Expand Down Expand Up @@ -272,7 +319,7 @@ describe(`nginx`, () => {

describe(`${host}/*`, () => {

it('should respond with 404 for unkown URLs (even if the resource exists)', done => {
it('should respond with 404 for unknown URLs (even if the resource exists)', done => {
['index.html', 'foo.js', 'foo/index.html'].forEach(relFilePath => {
const absFilePath = path.join(h.buildsDir, relFilePath);
h.writeFile(absFilePath, {content: `File: /${relFilePath}`});
Expand Down
2 changes: 1 addition & 1 deletion aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/server-integration.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ h.runForAllSupportedSchemes((scheme, port) => describe(`integration (on ${scheme
const archivePath = path.join(h.buildsDir, 'snapshot.tar.gz');

const getFile = (pr: string, sha: string, file: string) =>
h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha}.${host}/${file}`);
h.runCmd(`curl -iL ${scheme}://pr${pr}-${h.getShordSha(sha)}.${host}/${file}`);
const uploadBuild = (pr: string, sha: string, archive: string, authHeader = 'Token FOO') => {
// Using `FAKE_VERIFICATION_ERROR` or `FAKE_VERIFIED_NOT_TRUSTED` as `authHeader`,
// we can fake the response of the overwritten `BuildVerifier.verify()` method.
Expand Down
51 changes: 48 additions & 3 deletions aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,27 @@ describe('upload-server (on HTTP)', () => {
});


it('should not overwrite existing builds (even if the SHA is different)', done => {
// Since only the first few characters of the SHA are used, it is possible for two different
// SHAs to correspond to the same directory. In that case, we don't want the second SHA to
// overwrite the first.

const sha9Almost = sha9.replace(/.$/, '8');
expect(sha9Almost).not.toBe(sha9);

h.createDummyBuild(pr, sha9, isPublic);
expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toContain('index.html');

h.writeBuildFile(pr, sha9, 'index.html', 'My content', isPublic);
expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toBe('My content');

h.runCmd(`${cmdPrefix} http://${host}/create-build/${pr}/${sha9Almost}`).
then(h.verifyResponse(409, /^Request to overwrite existing directory/)).
then(() => expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toBe('My content')).
then(done);
});


it('should delete the PR directory on error (for new PR)', done => {
h.runCmd(`${cmdPrefix} http://${host}/create-build/${pr}/${sha9}`).
then(h.verifyResponse(500)).
Expand Down Expand Up @@ -175,7 +196,7 @@ describe('upload-server (on HTTP)', () => {

it(`should create files/directories owned by '${h.wwwUser}'`, done => {
const prDir = h.getPrDir(pr, isPublic);
const shaDir = path.join(prDir, sha9);
const shaDir = h.getShaDir(prDir, sha9);
const idxPath = path.join(shaDir, 'index.html');
const barPath = path.join(shaDir, 'foo', 'bar.js');

Expand Down Expand Up @@ -204,7 +225,7 @@ describe('upload-server (on HTTP)', () => {

it('should make the build directory non-writable', done => {
const prDir = h.getPrDir(pr, isPublic);
const shaDir = path.join(prDir, sha9);
const shaDir = h.getShaDir(prDir, sha9);
const idxPath = path.join(shaDir, 'index.html');
const barPath = path.join(shaDir, 'foo', 'bar.js');

Expand All @@ -224,6 +245,30 @@ describe('upload-server (on HTTP)', () => {
then(done);
});


it('should ignore a legacy 40-chars long build directory (even if it starts with the same chars)', done => {
// It is possible that 40-chars long build directories exist, if they had been deployed
// before implementing the shorter build directory names. In that case, we don't want the
// second (shorter) name to be considered the same as the old one (even if they originate
// from the same SHA).

h.createDummyBuild(pr, sha9, isPublic, false, true);
expect(h.readBuildFile(pr, sha9, 'index.html', isPublic, true)).toContain('index.html');

h.writeBuildFile(pr, sha9, 'index.html', 'My content', isPublic, true);
expect(h.readBuildFile(pr, sha9, 'index.html', isPublic, true)).toBe('My content');

h.runCmd(`${cmdPrefix} http://${host}/create-build/${pr}/${sha9}`).
then(h.verifyResponse(statusCode)).
then(() => {
expect(h.buildExists(pr, sha9, isPublic)).toBe(true);
expect(h.buildExists(pr, sha9, isPublic, true)).toBe(true);
expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toContain('index.html');
expect(h.readBuildFile(pr, sha9, 'index.html', isPublic, true)).toBe('My content');
}).
then(done);
});

});


Expand Down Expand Up @@ -277,7 +322,7 @@ describe('upload-server (on HTTP)', () => {


it('should reject the request if it fails to update the PR\'s visibility', done => {
// One way to cause an error is to have both a public and a hidden directory for the sme PR.
// One way to cause an error is to have both a public and a hidden directory for the same PR.
h.createDummyBuild(pr, sha0, isPublic);

expect(h.buildExists(pr, sha0, isPublic)).toBe(true);
Expand Down
Loading

0 comments on commit 4268c82

Please sign in to comment.