Skip to content

Proxy to masterPod instance if no DNS mapping #96

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
Myztiq merged 13 commits into from
Dec 3, 2015
Merged

Proxy to masterPod instance if no DNS mapping #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b3ba453
Proxy to masterPod instance if no DNS mapping
cflynn07 Nov 30, 2015
38a6077
Merge remote-tracking branch 'origin/master' into SAN-3023-referer-ro…
cflynn07 Nov 30, 2015
9345405
Add additional unit test for new logic
cflynn07 Nov 30, 2015
9b8497b
Fix naviEntry document selection for routing
cflynn07 Nov 30, 2015
fe94970
Update tests and masterPod default routeing logic
cflynn07 Dec 1, 2015
d9464a9
Use sinon assertion and stub API with _processTargetInstance stubs
cflynn07 Dec 1, 2015
c5288a3
Use sinon assertion and stub API with findMasterPodBranch stubs
cflynn07 Dec 1, 2015
6f5fc04
Use sinon assertion and stub API with findAssociationShortHashByElast…
cflynn07 Dec 1, 2015
df78307
Revert "Use sinon assertion and stub API with findAssociationShortHas…
cflynn07 Dec 1, 2015
fe9ef5a
Revert "Use sinon assertion and stub API with findMasterPodBranch stubs"
cflynn07 Dec 1, 2015
13b6038
Use sinon assertion and stub API with findAssociationShortHashByElast…
cflynn07 Dec 1, 2015
19c9319
Merge remote-tracking branch 'origin/master' into SAN-3023-referer-ro…
cflynn07 Dec 2, 2015
bf3feab
Use sinon assert match for method invocation argument testing
cflynn07 Dec 2, 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
33 changes: 20 additions & 13 deletions lib/models/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,25 +251,25 @@ api._getTargetHostElasticReferer = function (logData, naviEntry, req, next) {
var refererUserMappedInstanceId = (naviEntry.refererNaviEntry.userMappings) ?
naviEntry.refererNaviEntry.userMappings[req.session.userId] : null;
var refererNaviEntryInstance;
if (!refererUserMappedInstanceId) {
// no user-mapping for current user and referer instance, use master
if (refererUserMappedInstanceId) {
refererNaviEntryInstance =
mongo.constructor.findMasterPodBranch(naviEntry.refererNaviEntry);
naviEntry.refererNaviEntry.directUrls[refererUserMappedInstanceId];
log.trace(put({
refererUserMappedInstanceId: refererUserMappedInstanceId,
refererNaviEntryInstance: refererNaviEntryInstance
}, logData),
'_getTargetHostElasticReferer redis.lrange mongo.fetchNaviEntry isBrowser '+
'!refererNaviEntryInstance');
'refererNaviEntryInstance');
} else {
// no user-mapping for current user and referer instance, use master
refererNaviEntryInstance =
naviEntry.refererNaviEntry.directUrls[refererUserMappedInstanceId];
mongo.constructor.findMasterPodBranch(naviEntry.refererNaviEntry);
log.trace(put({
refererUserMappedInstanceId: refererUserMappedInstanceId,
refererNaviEntryInstance: refererNaviEntryInstance
}, logData),
'_getTargetHostElasticReferer redis.lrange mongo.fetchNaviEntry isBrowser '+
'refererNaviEntryInstance');
'!refererNaviEntryInstance');
}
if (!refererNaviEntryInstance) {
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 3, 2015

Choose a reason for hiding this comment

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

What if I have a referer coming from an instance that is destroyed, aka the user has not refreshed the page. Do we need to handle this?

log.error(logData,
Expand All @@ -281,14 +281,21 @@ api._getTargetHostElasticReferer = function (logData, naviEntry, req, next) {
var instanceShortHash = mongo.constructor.findAssociationShortHashByElasticUrl(
refererNaviEntryInstance.dependencies,
parsedReqUrl.hostname);
if (!instanceShortHash) {
// error because this shouldn't happen
log.error(
logData,
'_getTargetHostElasticReferer redis.lrange mongo.fetchNaviEntry isBrowser !instanceShortHash');
return next(ErrorCat.create(404, 'Not Found'));
if (instanceShortHash) {
log.trace(logData,
'getTargetHost redis.lrange mongo.fetchNaviEntry isBrowser instanceShortHash');
targetNaviEntryInstance = naviEntry.directUrls[instanceShortHash];
} else {
log.trace(logData,
'getTargetHost redis.lrange mongo.fetchNaviEntry isBrowser !instanceShortHash');

/**
* 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 = naviEntry.directUrls[instanceShortHash];

if (!targetNaviEntryInstance) {
// instance that referer naviEntry document was associated with isn't present,
// use master of requestUrl naviEntry masterPod instance
Expand Down
2 changes: 1 addition & 1 deletion lib/models/mongo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ Mongo.prototype.setUserMapping = function (elasticUrl, userId, instanceShortHash
/**
* Find nested directUrl object on a naviEntry document that is a masterPod branch
* @param {String} branchName
* @return {Object|undefined}
*/
Mongo.findMasterPodBranch = function (naviEntry) {
var logData = {
Expand All @@ -139,7 +140,6 @@ Mongo.findMasterPodBranch = function (naviEntry) {
}
}
log.warn(logData, 'findMasterPodBranch !match');
return null;
};

/**
Expand Down
59 changes: 42 additions & 17 deletions test/unit/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ describe('api.js unit test', function () {
describe('referer', function () {
var base = 'api-staging-codenow.runnableapp.com';
var req;

beforeEach(function (done) {
req = {
method: 'get',
Expand All @@ -391,16 +392,29 @@ describe('api.js unit test', function () {
sinon.stub(mongo, 'fetchNaviEntry', function (reqUrl, refererUrl, cb) {
cb(null, naviEntriesFixtures);
});

sinon.stub(api, '_processTargetInstance').yields();
sinon.stub(mongo.constructor, 'findAssociationShortHashByElasticUrl').returns(null);

done();
});

afterEach(function (done) {
api._getUrlFromRequest.restore();
redis.lrange.restore();
mongo.fetchNaviEntry.restore();
if (api._processTargetInstance.restore) {
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

No reason to do the if here. Since the beforeEach will always stub it.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

This is here because some of the methods under test assumed it wasn't stubbed... so some of the tests might have restored it themselves before the afterEach. Not ideal, I know.

api._processTargetInstance.restore();
}
if (mongo.constructor.findAssociationShortHashByElasticUrl.restore) {
mongo.constructor.findAssociationShortHashByElasticUrl.restore();
}
done();
});

it('should should ignore referer if same as requestUrl', function (done) {
api._processTargetInstance.restore();
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

This unit test should not be calling external libs. This restore is a bad thing here.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

same comment as above
``This is here because some of the methods under test assumed it wasn't stubbed... so some of the tests might have restored it themselves before the afterEach. Not ideal, I know.`

mongo.constructor.findAssociationShortHashByElasticUrl.restore();
req.headers.origin = 'http://'+base;
api.getTargetHost(req, {}, function (err) {
expect(err).to.be.undefined();
Expand All @@ -410,8 +424,9 @@ describe('api.js unit test', function () {
});
});


it('should proxy to instance mapped by referer naviEntry association', function (done) {
api._processTargetInstance.restore();
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

Same as above.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

same comment as above
`This is here because some of the methods under test assumed it wasn't stubbed... so some of the tests might have restored it themselves before the afterEach. Not ideal, I know.

mongo.constructor.findAssociationShortHashByElasticUrl.restore();
api.getTargetHost(req, {}, function (err) {
expect(err).to.be.undefined();
// feature-branch1 of API
Expand All @@ -421,6 +436,8 @@ describe('api.js unit test', function () {
});

it('should handle navientires document with no user-mappings', function (done) {
api._processTargetInstance.restore();
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

Same as above.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

same comment as above
`This is here because some of the methods under test assumed it wasn't stubbed... so some of the tests might have restored it themselves before the afterEach. Not ideal, I know.

mongo.constructor.findAssociationShortHashByElasticUrl.restore();
var restore = put({}, naviEntriesFixtures.refererNaviEntry);
delete naviEntriesFixtures.refererNaviEntry.userMappings;
api.getTargetHost(req, {}, function (err) {
Expand All @@ -433,6 +450,8 @@ describe('api.js unit test', function () {

it('should next with error if navientries document with no user-mappings and no '+
'masterpod', function (done) {
api._processTargetInstance.restore();
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

Same as above.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

same comment as above
`This is here because some of the methods under test assumed it wasn't stubbed... so some of the tests might have restored it themselves before the afterEach. Not ideal, I know.

mongo.constructor.findAssociationShortHashByElasticUrl.restore();
var restore = put({}, naviEntriesFixtures.refererNaviEntry);
delete naviEntriesFixtures.refererNaviEntry.userMappings;
naviEntriesFixtures.refererNaviEntry.directUrls.aaaaa1.masterPod = false;
Expand All @@ -444,31 +463,37 @@ describe('api.js unit test', function () {
});
});

it('should next with error if !instanceShortHash', function (done) {
sinon.stub(mongo.constructor, 'findAssociationShortHashByElasticUrl', function () {
return null;
it('should default to masterPod if !instanceShortHash', function (done) {
var mockNaviEntry = {};
sinon.stub(mongo.constructor, 'findMasterPodBranch', function () {
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.

can be done with sinon.stub(mongo.constructor, 'findMasterPodBranch).returns(mockNaviEntry)

Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

this stub should be in the forEach. Use Yields instead of passing a function.

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

Agreed but should we make this into a separate task? We could ship this bug fix with the tests that as Anand points out (it does meet the requirements and works)

return mockNaviEntry;
});

api.getTargetHost(req, {}, function (err) {
expect(err.message).to.equal('Not Found');
expect(err).to.be.undefined();
sinon.assert.calledWith(api._processTargetInstance, mockNaviEntry);
expect(mongo.constructor.findAssociationShortHashByElasticUrl.callCount).to.equal(1);
mongo.constructor.findAssociationShortHashByElasticUrl.restore();
mongo.constructor.findMasterPodBranch.restore();
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

Restore should only be done in afterEach.

done();
});
});

it('should default to masterPod instance if assocation not in requestUrl directUrls',
function (done) {
sinon.stub(mongo.constructor, 'findAssociationShortHashByElasticUrl', function () {
return 'FFFFF'; //This is an instance not defined in requestUrl directUrls
});
sinon.stub(api, '_processTargetInstance',
function (targetNaviEntryInstance, reqUrl, req, next) {
expect(targetNaviEntryInstance.masterPod).to.equal(true);
mongo.constructor.findAssociationShortHashByElasticUrl.restore();
api._processTargetInstance.restore();
next();
it('should default to masterPod instance if no associations/dns-mappings defined',
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Nov 30, 2015

Choose a reason for hiding this comment

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

It's really weird to stub when not in a beforeEach because cleanup should be in an afterEach so if the test failed it still gets cleaned up

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

Many of these tests need to stub these methods to return different values, which I can't really do cleanly in a beforeEach

Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

if you need to stub it for different values you can stub it in one spot and then just do x.bar.yields(myNewVal) instead of sinon.stub(x, 'bar').yields(myNewVal) everywhere.

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.

+1 I have used that pattern a lot

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

Sounds good I'll do that from now on

Copy link
Copy Markdown
Contributor Author

@cflynn07 cflynn07 Dec 1, 2015

Choose a reason for hiding this comment

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

Moved this into a beforeEach/afterEach

Copy link
Copy Markdown
Contributor

@Myztiq Myztiq Dec 1, 2015

Choose a reason for hiding this comment

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

This test has zero assertions.

function (done) {

sinon.stub(mongo.constructor, 'findMasterPodBranch');
mongo.constructor.findMasterPodBranch.onFirstCall().returns({});
mongo.constructor.findMasterPodBranch.onSecondCall().returns(undefined);
mongo.constructor.findMasterPodBranch.onThirdCall().returns({
masterPod: true
});

api.getTargetHost(req, {}, function () {
sinon.assert.calledWith(api._processTargetInstance, sinon.match({
masterPod: true
}));
sinon.assert.calledWith(api._processTargetInstance)
mongo.constructor.findMasterPodBranch.restore();
done();
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/unit/mongo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ describe('lib/models/mongodb', function () {
var copy = put({}, naviEntryFixtures);
copy.directUrls = {};
var masterPod = mongo.constructor.findMasterPodBranch(copy);
expect(masterPod).to.equal(null);
expect(masterPod).to.be.undefined();
done();
});
});
Expand Down