Skip to content

Refactor construction of error-page proxy url to detention #94

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.

Sign up for GitHub

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

Merged
cflynn07 merged 20 commits into from
Dec 5, 2015
Merged

Refactor construction of error-page proxy url to detention #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
23dae97
Change function invocation to pass additional shortHash argument
cflynn07 Nov 26, 2015
901b991
Fix reference errors in api module
cflynn07 Nov 26, 2015
36bca3d
Update unit test stubs and expects
cflynn07 Nov 26, 2015
60a08df
Fix referer entry masterPod lookup
cflynn07 Nov 26, 2015
9d8369a
Update error-page unit test expectations
cflynn07 Nov 26, 2015
c23a0eb
Update mongo unit test expects
cflynn07 Nov 26, 2015
52ad014
Add timing debug logs
cflynn07 Nov 30, 2015
8dff5a0
Add properties to req object for erro page generation
cflynn07 Nov 30, 2015
0093ccc
Add comment for Anand review
cflynn07 Nov 30, 2015
85438b3
Tweak code comment
cflynn07 Nov 30, 2015
689977a
Update trace log metadata
cflynn07 Nov 30, 2015
a78789b
Remove timing logs
cflynn07 Nov 30, 2015
cafbbee
Merge remote-tracking branch 'origin/master' into SAN-3006-refactor-d…
cflynn07 Dec 2, 2015
c1f756b
Update jsdoc comment function return description
cflynn07 Dec 2, 2015
59154ed
Fix minor whitespace
cflynn07 Dec 2, 2015
228f9f9
Revert "Update trace log metadata"
cflynn07 Dec 2, 2015
82e600d
Add descriptive comment of proxy edgecase logic
cflynn07 Dec 2, 2015
1ad7d69
Add unit test for api._processTargetInstance
cflynn07 Dec 2, 2015
2717f42
Merge remote-tracking branch 'origin/master' into SAN-3006-refactor-d…
cflynn07 Dec 3, 2015
f604255
Remove code comments
cflynn07 Dec 3, 2015
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
68 changes: 46 additions & 22 deletions lib/models/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,18 @@ api._shouldBypassAuth = function (req) {
/**
* Proxy request to detention if container is not running.
* Proxy request to container is running
* @param {Object} targetNaviEntryInstance
* @param {String} shortHash
* @param {String} reqUrl
* @param {Object} req
* @param {Function} next
*/
api._processTargetInstance = function (targetNaviEntryInstance, reqUrl, req, next) {
api._processTargetInstance = function (targetNaviEntryInstance, shortHash, reqUrl, req, next) {
var logData = {
tx: true,
targetNaviEntryInstance: targetNaviEntryInstance,
reqUrl: reqUrl
reqUrl: reqUrl,
shortHash: shortHash
};
log.info(logData, 'api._processTargetInstance');

Expand All @@ -127,14 +133,17 @@ api._processTargetInstance = function (targetNaviEntryInstance, reqUrl, req, nex
return next(ErrorCat.create(404, 'Not Found'));
}

// Adding these to req object to make values available in proxy module's error event handler.
req.shortHash = shortHash;
req.elasticUrl = reqUrl;

// for error-page module if container is unresponsive
req.targetBranch = targetNaviEntryInstance.branch;

if (!targetNaviEntryInstance.running) {
log.trace(logData, '_processTargetInstance !running');
req.targetHost = errorPage.generateErrorUrl('not_running', {
elasticUrl: reqUrl,
targetBranch: targetNaviEntryInstance.branch
shortHash: shortHash
});
return next();
}
Expand Down Expand Up @@ -194,29 +203,35 @@ api._getTargetHostElastic = function (logData, req, next) {
log.trace(logData,
'_getTargetHostElastic redis.lrange mongo.fetchNaviEntry isBrowser !refererUrl');
// document might not have a userMappings key yet
var mappedInstanceId = (naviEntry.userMappings) ?
var mappedInstanceShortHash = (naviEntry.userMappings) ?
naviEntry.userMappings[req.session.userId] : null;
if (!mappedInstanceId) {
if (!mappedInstanceShortHash) {
log.trace(logData,
'_getTargetHostElastic redis.lrange mongo.fetchNaviEntry isBrowser !refererUrl '+
'!mappedInstanceId');
'!mappedInstanceShortHash');
// use master
targetNaviEntryInstance = mongo.constructor.findMasterPodBranch(naviEntry);
var findResult = mongo.constructor.findMasterPodBranch(naviEntry);
Copy link
Copy Markdown
Contributor

@anandkumarpatel anandkumarpatel Dec 1, 2015

Choose a reason for hiding this comment

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

in the other PR you check for shorthash, do you need to do same check here?
https://github.com/CodeNow/navi/pull/96/files#diff-7ccf25494e1cbc5cba89bfd84e901eb9R293

+    var findResult = mongo.constructor.findMasterPodBranch(naviEntry);
+    instanceShortHash = keypather.get(findResult, 'directUrlShortHash');
+    if (!instanceShortHash) {
+      log.warn(
+        logData,
+        'getTargetHost redis.lrange mongo.fetchNaviEntry isBrowser !instanceShortHash masterpod');
+      return next(ErrorCat.create(404, 'Not Found'));
+    }

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 2, 2015

Choose a reason for hiding this comment

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

Not here, our data * should * never exist in a state without 1 masterPodBranch. That other PR is checking for the event that the referer header is a valid formatted runnable elastic url but isn't actually a url that the target instance has an association with. Different situation. link you shared - that's an acceptable state that can occur.

targetNaviEntryInstance = findResult.directUrlObj;
mappedInstanceShortHash = findResult.directUrlShortHash;
} else {
log.trace(put({
mappedInstanceId: mappedInstanceId
mappedInstanceShortHash: mappedInstanceShortHash
}, logData),
'_getTargetHostElastic redis.lrange mongo.fetchNaviEntry isBrowser !refererUrl '+
'mappedInstanceId');
targetNaviEntryInstance = naviEntry.directUrls[mappedInstanceId];
'mappedInstanceShortHash');
targetNaviEntryInstance = naviEntry.directUrls[mappedInstanceShortHash];
}
return api._processTargetInstance(targetNaviEntryInstance, reqUrl, req, next);
return api._processTargetInstance(targetNaviEntryInstance, mappedInstanceShortHash,
reqUrl, req, next);
}
} else {
// if not browser, proxy to master
log.trace(logData, '_getTargetHostElastic redis.lrange mongo.fetchNaviEntry !isBrowser');
targetNaviEntryInstance = mongo.constructor.findMasterPodBranch(naviEntry);
return api._processTargetInstance(targetNaviEntryInstance, reqUrl, req, next);
log.trace(logData, 'getTargetHost redis.lrange mongo.fetchNaviEntry !isBrowser');
var findResult = mongo.constructor.findMasterPodBranch(naviEntry);
Copy link
Copy Markdown
Contributor

@anandkumarpatel anandkumarpatel Dec 1, 2015

Choose a reason for hiding this comment

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

in the other PR you check for shorthash, do you need to do same check here?
https://github.com/CodeNow/navi/pull/96/files#diff-7ccf25494e1cbc5cba89bfd84e901eb9R293

+    var findResult = mongo.constructor.findMasterPodBranch(naviEntry);
+    instanceShortHash = keypather.get(findResult, 'directUrlShortHash');
+    if (!instanceShortHash) {
+      log.warn(
+        logData,
+        'getTargetHost redis.lrange mongo.fetchNaviEntry isBrowser !instanceShortHash masterpod');
+      return next(ErrorCat.create(404, 'Not Found'));
+    }

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 2, 2015

Choose a reason for hiding this comment

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

Same answer as above so I copy/paste:

Not here, our data * should * never exist in a state without 1 masterPodBranch. That other PR is checking for the event that the referer header is a valid formatted runnable elastic url but isn't actually a url that the target instance has an association with. Different situation. link you shared - that's an acceptable state that can occur.

targetNaviEntryInstance = findResult.directUrlObj;
return api._processTargetInstance(targetNaviEntryInstance,
findResult.directUrlShortHash,
reqUrl, req, next);
}
});
};
Expand Down Expand Up @@ -251,9 +266,9 @@ api._getTargetHostElasticReferer = function (logData, naviEntry, req, next) {
var refererUserMappedInstanceId = (naviEntry.refererNaviEntry.userMappings) ?
naviEntry.refererNaviEntry.userMappings[req.session.userId] : null;
var refererNaviEntryInstance;

if (refererUserMappedInstanceId) {
refererNaviEntryInstance =
naviEntry.refererNaviEntry.directUrls[refererUserMappedInstanceId];
refererNaviEntryInstance = naviEntry.refererNaviEntry.directUrls[refererUserMappedInstanceId];
log.trace(put({
refererUserMappedInstanceId: refererUserMappedInstanceId,
refererNaviEntryInstance: refererNaviEntryInstance
Expand All @@ -262,8 +277,9 @@ api._getTargetHostElasticReferer = function (logData, naviEntry, req, next) {
'refererNaviEntryInstance');
} else {
// no user-mapping for current user and referer instance, use master
refererNaviEntryInstance =
mongo.constructor.findMasterPodBranch(naviEntry.refererNaviEntry);
var findResult = mongo.constructor.findMasterPodBranch(naviEntry.refererNaviEntry);
refererNaviEntryInstance = keypather.get(findResult, 'directUrlObj');
refererUserMappedInstanceId = keypather.get(findResult, 'directUrlShortHash');
log.trace(put({
refererUserMappedInstanceId: refererUserMappedInstanceId,
refererNaviEntryInstance: refererNaviEntryInstance
Expand Down Expand Up @@ -293,7 +309,9 @@ api._getTargetHostElasticReferer = function (logData, naviEntry, req, next) {
* Referer url is a valid runnable elasticUrl however there might be no defined DNS mappings.
* Proxy to the masterPod instance if so.
*/
targetNaviEntryInstance = mongo.constructor.findMasterPodBranch(naviEntry);
//targetNaviEntryInstance = mongo.constructor.findMasterPodBranch(naviEntry);
var findResult = mongo.constructor.findMasterPodBranch(naviEntry.refererNaviEntry);
targetNaviEntryInstance = keypather.get(findResult, 'directUrlObj');
}

if (!targetNaviEntryInstance) {
Expand All @@ -304,9 +322,15 @@ api._getTargetHostElasticReferer = function (logData, naviEntry, req, next) {
}, logData),
'_getTargetHostElasticReferer redis.lrange mongo.fetchNaviEntry isBrowser !targetNaviEntryInstance '+
'use master');
targetNaviEntryInstance = mongo.constructor.findMasterPodBranch(naviEntry);

var findResult = mongo.constructor.findMasterPodBranch(naviEntry);
Copy link
Copy Markdown
Contributor

@anandkumarpatel anandkumarpatel Dec 1, 2015

Choose a reason for hiding this comment

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

same note

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 2, 2015

Choose a reason for hiding this comment

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

same answer

targetNaviEntryInstance = findResult.directUrlObj;
return api._processTargetInstance(targetNaviEntryInstance,
findResult.directUrlShortHash,
reqUrl, req, next);

}
return api._processTargetInstance(targetNaviEntryInstance, reqUrl, req, next);
return api._processTargetInstance(targetNaviEntryInstance, instanceShortHash, reqUrl, req, next);
};

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/models/error-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function generateErrorUrl(type, opts) {
log.trace(logData, 'generateErrorUrl signin');
} else if (type === 'not_running' || type === 'unresponsive' || type === 'ports') {
query.elasticUrl = opts.elasticUrl;
query.targetBranch = opts.targetBranch;
query.shortHash = opts.shortHash;
} else {
throw ErrorCat.createAndReport(500, 'invalid error page');
}
Expand Down
8 changes: 6 additions & 2 deletions lib/models/mongo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ Mongo.prototype.setUserMapping = function (elasticUrl, userId, instanceShortHash
};

/**
* Find nested directUrl object on a naviEntry document that is a masterPod branch
* Find nested directUrl object and its key (shortHash on a naviEntry document that is a masterPod
* branch
* @param {String} branchName
* @return {Object|undefined}
*/
Expand All @@ -136,7 +137,10 @@ Mongo.findMasterPodBranch = function (naviEntry) {
for (var i = 0, len = shortHashes.length; i < len; i++) {
var directUrlObj = naviEntry.directUrls[shortHashes[i]];
if (directUrlObj.masterPod) {
return directUrlObj;
return {
Copy link
Copy Markdown
Contributor

@anandkumarpatel anandkumarpatel Dec 1, 2015

Choose a reason for hiding this comment

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

update comment above saying what this returns.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 2, 2015

Choose a reason for hiding this comment

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

good catch

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 2, 2015

Choose a reason for hiding this comment

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

done

directUrlShortHash: shortHashes[i],
directUrlObj: directUrlObj
};
}
}
log.warn(logData, 'findMasterPodBranch !match');
Expand Down
21 changes: 12 additions & 9 deletions lib/models/proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ function Proxy () {
log.info('Proxy constructor');
var self = this;
this.proxy = httpProxy.createProxyServer({});
// setup error handling
this.proxy.on('error', function (err, req/*, proxiedRes */) {
var res = req.res;
var logData = {
Expand All @@ -44,11 +43,16 @@ function Proxy () {
req: req
};
log.error(logData, 'proxy.on error');
/**
* we need to keep the didError logic. sometimes (not sure why) but we try to proxy the same
* request twice (in some network cases) and navi will crash saying you can't send on a request
* that is already ended
*/
if (!req.didError) {
req.didError = true;
var errorUrl = errorPage.generateErrorUrl('unresponsive', {
elasticUrl: '',
targetBranch: ''
shortHash: req.shortHash,
elasticUrl: req.elasticUrl
});
var targetHost = getHostAndAugmentReq(req, errorUrl);
log.trace(put({
Expand Down Expand Up @@ -165,12 +169,11 @@ Proxy.prototype._streamRes = function (targetRes, proxiedRes, res) {
log.trace(logData, '_streamRes resIsHtml');
delete targetRes.headers['content-length'];
var scriptInjectStream = scriptInjectResStreamFactory.create(xhrPatchScript, resIsGziped);
proxiedRes
.pipe(scriptInjectStream.input);
scriptInjectStream.output
.pipe(res);
}
else {
proxiedRes.pipe(scriptInjectStream.input);

// Added these logs to debug timing of script injection
scriptInjectStream.output.pipe(res);
} else {
log.trace(logData, '_streamRes !resIsHtml');
// if the response type is not html transformRes should not modify the response
// finally pipe target response data to the real response
Expand Down
56 changes: 47 additions & 9 deletions test/unit/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var lab = exports.lab = Lab.script();

//var errorPage = require('models/error-page.js');
var api = require('models/api.js');
var log = require('middlewares/logger')(__filename).log;
var mongo = require('models/mongo');
var naviEntriesFixtures = require('../fixture/navi-entries');
var naviRedisEntriesFixture = require('../fixture/navi-redis-entries');
Expand Down Expand Up @@ -148,6 +149,7 @@ describe('api.js unit test', function () {
expect(test).to.equal(result);
done();
});

it('should add https', function (done) {
var test = api._getUrlFromRequest({
isBrowser: true,
Expand Down Expand Up @@ -219,23 +221,47 @@ describe('api.js unit test', function () {
});

describe('_processTargetInstance', function () {
beforeEach(function (done) {
sinon.stub(api, '_getDestinationProxyUrl');
done();
});

afterEach(function (done) {
api._getDestinationProxyUrl.restore();
done();
});

it('should next with error if !targetNaviEntryInstance', function (done) {
api._processTargetInstance(null, '', {}, function (err) {
api._processTargetInstance(null, '', '', {}, function (err) {
Copy link
Copy Markdown
Contributor

@bkendall bkendall Dec 1, 2015

Choose a reason for hiding this comment

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

this addition makes me think "is there a test for if this is missing or invalid?". I don't see one in the near vicinity...

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 2, 2015

Choose a reason for hiding this comment

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

@bkendall I just made sure api._processTargetInstance is fully covered by the unit tests. Added one more test for it. 1ad7d69

expect(err.message).to.equal('Not Found');
done();
});
});

it('should set req.targetHost if !running', function (done) {
it('should set req.targetHost to error url if !running', function (done) {
var req = {};
var reqUrl = 'api-staging-codenow.runnableapp.com';
api._processTargetInstance({
running: false,
branch: 'master'
}, reqUrl, req, function (err) {
}, '55555', reqUrl, req, function (err) {
expect(err).to.be.undefined();
expect(req.targetHost).to.equal('http://localhost:55551?type=not_running&elasticUrl='+
reqUrl+'&targetBranch=master');
reqUrl+'&shortHash=55555');
done();
});
});

it('should set req.targetHost to container host & port if running', function (done) {
api._getDestinationProxyUrl.returns('http://0.0.0.0:600');
var req = {};
var reqUrl = 'api-staging-codenow.runnableapp.com';
api._processTargetInstance({
running: true,
branch: 'master'
}, '55555', reqUrl, req, function (err) {
expect(err).to.be.undefined();
expect(req.targetHost).to.equal('http://0.0.0.0:600');
done();
});
});
Expand Down Expand Up @@ -438,35 +464,45 @@ describe('api.js unit test', function () {
it('should handle navientires document with no user-mappings', function (done) {
api._processTargetInstance.restore();
mongo.constructor.findAssociationShortHashByElasticUrl.restore();

var restore = put({}, naviEntriesFixtures.refererNaviEntry);
delete naviEntriesFixtures.refererNaviEntry.userMappings;

api.getTargetHost(req, {}, function (err) {
expect(err).to.be.undefined();
expect(req.targetHost).to.equal('http://0.0.0.2:39942');
naviEntriesFixtures.refererNaviEntry.userMappings = restore;
naviEntriesFixtures.refererNaviEntry = restore;
done();
});
});

it('should next with error if navientries document with no user-mappings and no '+
'masterpod', function (done) {

api._processTargetInstance.restore();
mongo.constructor.findAssociationShortHashByElasticUrl.restore();

var restore = put({}, naviEntriesFixtures.refererNaviEntry);
delete naviEntriesFixtures.refererNaviEntry.userMappings;
naviEntriesFixtures.refererNaviEntry.directUrls.aaaaa1.masterPod = false;

api.getTargetHost(req, {}, function (err) {
expect(err.message).to.equal('Not Found');
naviEntriesFixtures.refererNaviEntry.userMappings = restore;
naviEntriesFixtures.refererNaviEntry.directUrls.aaaaa1.masterPod = true;

naviEntriesFixtures.refererNaviEntry = restore;

done();
});
});

it('should default to masterPod if !instanceShortHash', function (done) {
var mockNaviEntry = {};
sinon.stub(mongo.constructor, 'findMasterPodBranch', function () {
return mockNaviEntry;

sinon.stub(mongo.constructor, 'findMasterPodBranch').returns({
directUrlObj: mockNaviEntry,
directUrlShortHash: 'FFFF'
});

api.getTargetHost(req, {}, function (err) {
Expand All @@ -484,8 +520,10 @@ describe('api.js unit test', function () {
sinon.stub(mongo.constructor, 'findMasterPodBranch');
mongo.constructor.findMasterPodBranch.onFirstCall().returns({});
mongo.constructor.findMasterPodBranch.onSecondCall().returns(undefined);
mongo.constructor.findMasterPodBranch.onThirdCall().returns({
masterPod: true
mongo.constructor.findMasterPodBranch.onSecondCall().returns({
directUrlObj: {
masterPod: true
}
});

api.getTargetHost(req, {}, function () {
Expand Down
Loading