From db93fa3a8b52378ebda88426be7112ad8c4a90f7 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Thu, 21 Jan 2021 07:20:55 -0500 Subject: [PATCH] Merge pull request #3556 from overleaf/jpa-clsi-persistance-query-param [CompileController] enable clsi node persistence via query parameter GitOrigin-RevId: 515814d6ad5832e69538ef6d63f81c61c66fd73f --- .../app/src/Features/Compile/ClsiManager.js | 107 +++++++----- .../src/Features/Compile/CompileController.js | 47 ++++-- .../src/Features/Compile/CompileManager.js | 21 ++- services/web/app/src/router.js | 3 + .../test/unit/src/Compile/ClsiManagerTests.js | 157 ++++++++++++++---- .../src/Compile/CompileControllerTests.js | 10 +- 6 files changed, 254 insertions(+), 91 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 5e50f9e2b4..8ee7308e86 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -136,7 +136,7 @@ const ClsiManager = { ClsiManager._makeRequest(projectId, opts, callback) }, - deleteAuxFiles(projectId, userId, options, callback) { + deleteAuxFiles(projectId, userId, options, clsiserverid, callback) { if (options == null) { options = {} } @@ -149,27 +149,32 @@ const ClsiManager = { url: compilerUrl, method: 'DELETE' } - ClsiManager._makeRequest(projectId, opts, clsiErr => { - // always clear the project state from the docupdater, even if there - // was a problem with the request to the clsi - DocumentUpdaterHandler.clearProjectState(projectId, docUpdaterErr => { - if (clsiErr != null) { - return callback( - OError.tag(clsiErr, 'Failed to delete aux files', { projectId }) - ) - } - if (docUpdaterErr != null) { - return callback( - OError.tag( - docUpdaterErr, - 'Failed to clear project state in doc updater', - { projectId } + ClsiManager._makeRequestWithClsiServerId( + projectId, + opts, + clsiserverid, + clsiErr => { + // always clear the project state from the docupdater, even if there + // was a problem with the request to the clsi + DocumentUpdaterHandler.clearProjectState(projectId, docUpdaterErr => { + if (clsiErr != null) { + return callback( + OError.tag(clsiErr, 'Failed to delete aux files', { projectId }) ) - ) - } - callback() - }) - }) + } + if (docUpdaterErr != null) { + return callback( + OError.tag( + docUpdaterErr, + 'Failed to clear project state in doc updater', + { projectId } + ) + ) + } + callback() + }) + } + ) }, _sendBuiltRequest(projectId, userId, req, options, callback) { @@ -253,6 +258,23 @@ const ClsiManager = { ) }, + _makeRequestWithClsiServerId(projectId, opts, clsiserverid, callback) { + if (clsiserverid) { + // ignore cookies and newBackend, go straight to the clsi node + opts.qs = Object.assign({ clsiserverid }, opts.qs) + request(opts, (err, response, body) => { + if (err) { + return callback( + OError.tag(err, 'error making request to CLSI', { projectId }) + ) + } + callback(null, response, body) + }) + } else { + ClsiManager._makeRequest(projectId, opts, callback) + } + }, + _makeRequest(projectId, opts, callback) { async.series( { @@ -787,7 +809,7 @@ const ClsiManager = { }) }, - wordCount(projectId, userId, file, options, callback) { + wordCount(projectId, userId, file, options, clsiserverid, callback) { ClsiManager._buildRequest(projectId, options, (err, req) => { if (err != null) { return callback( @@ -812,25 +834,32 @@ const ClsiManager = { }, method: 'GET' } - ClsiManager._makeRequest(projectId, opts, (err, response, body) => { - if (err != null) { - return callback(OError.tag(err, 'CLSI request failed', { projectId })) - } - if (response.statusCode >= 200 && response.statusCode < 300) { - callback(null, body) - } else { - callback( - new OError( - `CLSI returned non-success code: ${response.statusCode}`, - { - projectId, - clsiResponse: body, - statusCode: response.statusCode - } + ClsiManager._makeRequestWithClsiServerId( + projectId, + opts, + clsiserverid, + (err, response, body) => { + if (err != null) { + return callback( + OError.tag(err, 'CLSI request failed', { projectId }) ) - ) + } + if (response.statusCode >= 200 && response.statusCode < 300) { + callback(null, body) + } else { + callback( + new OError( + `CLSI returned non-success code: ${response.statusCode}`, + { + projectId, + clsiResponse: body, + statusCode: response.statusCode + } + ) + ) + } } - }) + ) }) } } diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 85e3bffb8a..6c678af918 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -255,11 +255,12 @@ module.exports = CompileController = { deleteAuxFiles(req, res, next) { const project_id = req.params.Project_id + const { clsiserverid } = req.query return CompileController._compileAsUser(req, function(error, user_id) { if (error != null) { return next(error) } - return CompileManager.deleteAuxFiles(project_id, user_id, function( + CompileManager.deleteAuxFiles(project_id, user_id, clsiserverid, function( error ) { if (error != null) { @@ -465,7 +466,7 @@ module.exports = CompileController = { if (next == null) { next = function(error) {} } - return ClsiCookieManager.getCookieJar(project_id, function(err, jar) { + _getPersistenceOptions(req, project_id, (err, persistenceOptions) => { let qs if (err != null) { OError.tag(err, 'error getting cookie jar for clsi request') @@ -479,10 +480,15 @@ module.exports = CompileController = { url = `${compilerUrl}${url}` const oneMinute = 60 * 1000 // the base request - const options = { url, method: req.method, timeout: oneMinute, jar } + const options = { + url, + method: req.method, + timeout: oneMinute, + ...persistenceOptions + } // add any provided query string if (qs != null) { - options.qs = qs + options.qs = Object.assign(options.qs, qs) } // if we have a build parameter, pass it through to the clsi if ( @@ -518,20 +524,35 @@ module.exports = CompileController = { wordCount(req, res, next) { const project_id = req.params.Project_id const file = req.query.file || false + const { clsiserverid } = req.query return CompileController._compileAsUser(req, function(error, user_id) { if (error != null) { return next(error) } - return CompileManager.wordCount(project_id, user_id, file, function( - error, - body - ) { - if (error != null) { - return next(error) + CompileManager.wordCount( + project_id, + user_id, + file, + clsiserverid, + function(error, body) { + if (error != null) { + return next(error) + } + res.contentType('application/json') + return res.send(body) } - res.contentType('application/json') - return res.send(body) - }) + ) + }) + } +} + +function _getPersistenceOptions(req, projectId, callback) { + const { clsiserverid } = req.query + if (clsiserverid && typeof clsiserverid === 'string') { + callback(null, { qs: { clsiserverid } }) + } else { + ClsiCookieManager.getCookieJar(projectId, (err, jar) => { + callback(err, { jar }) }) } } diff --git a/services/web/app/src/Features/Compile/CompileManager.js b/services/web/app/src/Features/Compile/CompileManager.js index ece25216b2..24f704b336 100644 --- a/services/web/app/src/Features/Compile/CompileManager.js +++ b/services/web/app/src/Features/Compile/CompileManager.js @@ -136,7 +136,7 @@ module.exports = CompileManager = { }) }, - deleteAuxFiles(project_id, user_id, callback) { + deleteAuxFiles(project_id, user_id, clsiserverid, callback) { if (callback == null) { callback = function(error) {} } @@ -147,7 +147,13 @@ module.exports = CompileManager = { if (error != null) { return callback(error) } - return ClsiManager.deleteAuxFiles(project_id, user_id, limits, callback) + ClsiManager.deleteAuxFiles( + project_id, + user_id, + limits, + clsiserverid, + callback + ) }) }, @@ -253,7 +259,7 @@ module.exports = CompileManager = { }) }, - wordCount(project_id, user_id, file, callback) { + wordCount(project_id, user_id, file, clsiserverid, callback) { if (callback == null) { callback = function(error) {} } @@ -264,7 +270,14 @@ module.exports = CompileManager = { if (error != null) { return callback(error) } - return ClsiManager.wordCount(project_id, user_id, file, limits, callback) + ClsiManager.wordCount( + project_id, + user_id, + file, + limits, + clsiserverid, + callback + ) }) } } diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index f9c9a2ffac..c4aeec3564 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -49,6 +49,7 @@ const TemplatesRouter = require('./Features/Templates/TemplatesRouter') const InstitutionsController = require('./Features/Institutions/InstitutionsController') const UserMembershipRouter = require('./Features/UserMembership/UserMembershipRouter') const SystemMessageController = require('./Features/SystemMessages/SystemMessageController') +const { Joi, validate } = require('./infrastructure/Validation') const logger = require('logger-sharelatex') const _ = require('underscore') @@ -445,6 +446,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.delete( '/project/:Project_id/output', + validate({ query: { clsiserverid: Joi.string() } }), AuthorizationMiddleware.ensureUserCanReadProject, CompileController.deleteAuxFiles ) @@ -460,6 +462,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) webRouter.get( '/project/:Project_id/wordcount', + validate({ query: { clsiserverid: Joi.string() } }), AuthorizationMiddleware.ensureUserCanReadProject, CompileController.wordCount ) diff --git a/services/web/test/unit/src/Compile/ClsiManagerTests.js b/services/web/test/unit/src/Compile/ClsiManagerTests.js index 3b32d31272..923d6bf5ec 100644 --- a/services/web/test/unit/src/Compile/ClsiManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiManagerTests.js @@ -391,7 +391,7 @@ describe('ClsiManager', function() { describe('deleteAuxFiles', function() { beforeEach(function() { - this.ClsiManager._makeRequest = sinon.stub().callsArg(2) + this.ClsiManager._makeRequestWithClsiServerId = sinon.stub().yields(null) this.DocumentUpdaterHandler.clearProjectState = sinon.stub().callsArg(1) }) @@ -401,16 +401,21 @@ describe('ClsiManager', function() { this.project_id, this.user_id, { compileGroup: 'standard' }, + 'node-1', this.callback ) }) it('should call the delete method in the standard CLSI', function() { - this.ClsiManager._makeRequest - .calledWith(this.project_id, { - method: 'DELETE', - url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}` - }) + this.ClsiManager._makeRequestWithClsiServerId + .calledWith( + this.project_id, + { + method: 'DELETE', + url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}` + }, + 'node-1' + ) .should.equal(true) }) @@ -888,14 +893,9 @@ describe('ClsiManager', function() { describe('wordCount', function() { beforeEach(function() { - this.ClsiManager._makeRequest = sinon + this.ClsiManager._makeRequestWithClsiServerId = sinon .stub() - .callsArgWith( - 2, - null, - { statusCode: 200 }, - (this.body = { mock: 'foo' }) - ) + .yields(null, { statusCode: 200 }, (this.body = { mock: 'foo' })) this.ClsiManager._buildRequest = sinon.stub().callsArgWith( 2, null, @@ -912,17 +912,25 @@ describe('ClsiManager', function() { this.user_id, false, {}, + 'node-1', this.callback ) }) it('should call wordCount with root file', function() { - this.ClsiManager._makeRequest - .calledWith(this.project_id, { - method: 'GET', - url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, - qs: { file: 'rootfile.text', image: undefined } - }) + this.ClsiManager._makeRequestWithClsiServerId + .calledWith( + this.project_id, + { + method: 'GET', + url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, + qs: { + file: 'rootfile.text', + image: undefined + } + }, + 'node-1' + ) .should.equal(true) }) @@ -938,17 +946,22 @@ describe('ClsiManager', function() { this.user_id, 'main.tex', {}, + 'node-2', this.callback ) }) it('should call wordCount with param file', function() { - this.ClsiManager._makeRequest - .calledWith(this.project_id, { - method: 'GET', - url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, - qs: { file: 'main.tex', image: undefined } - }) + this.ClsiManager._makeRequestWithClsiServerId + .calledWith( + this.project_id, + { + method: 'GET', + url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, + qs: { file: 'main.tex', image: undefined } + }, + 'node-2' + ) .should.equal(true) }) }) @@ -962,17 +975,22 @@ describe('ClsiManager', function() { this.user_id, 'main.tex', {}, + 'node-3', this.callback ) }) it('should call wordCount with file and image', function() { - this.ClsiManager._makeRequest - .calledWith(this.project_id, { - method: 'GET', - url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, - qs: { file: 'main.tex', image: this.image } - }) + this.ClsiManager._makeRequestWithClsiServerId + .calledWith( + this.project_id, + { + method: 'GET', + url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, + qs: { file: 'main.tex', image: this.image } + }, + 'node-3' + ) .should.equal(true) }) }) @@ -1008,6 +1026,83 @@ describe('ClsiManager', function() { }) }) + describe('_makeRequestWithClsiServerId', function() { + beforeEach(function() { + this.response = { statusCode: 200 } + this.request.yields(null, this.response) + this.opts = { + method: 'GET', + url: 'http://clsi' + } + }) + + describe('with a regular request', function() { + it('should process a request with a cookie jar', function(done) { + this.ClsiManager._makeRequestWithClsiServerId( + this.project_id, + this.opts, + undefined, + err => { + if (err) return done(err) + const args = this.request.args[0] + args[0].method.should.equal(this.opts.method) + args[0].url.should.equal(this.opts.url) + args[0].jar.should.equal(this.jar) + expect(args[0].qs).to.not.exist + done() + } + ) + }) + + it('should persist the cookie from the response', function(done) { + this.ClsiManager._makeRequestWithClsiServerId( + this.project_id, + this.opts, + undefined, + err => { + if (err) return done(err) + this.ClsiCookieManager.setServerId + .calledWith(this.project_id, this.response) + .should.equal(true) + done() + } + ) + }) + }) + + describe('with a persistent request', function() { + it('should not add a cookie jar', function(done) { + this.ClsiManager._makeRequestWithClsiServerId( + this.project_id, + this.opts, + 'node-1', + err => { + if (err) return done(err) + const requestOpts = this.request.args[0][0] + expect(requestOpts.method).to.equal(this.opts.method) + expect(requestOpts.url).to.equal(this.opts.url) + expect(requestOpts.jar).to.not.exist + expect(requestOpts.qs).to.deep.equal({ clsiserverid: 'node-1' }) + done() + } + ) + }) + + it('should not persist a cookie on response', function(done) { + this.ClsiManager._makeRequestWithClsiServerId( + this.project_id, + this.opts, + 'node-1', + err => { + if (err) return done(err) + expect(this.ClsiCookieManager.setServerId.called).to.equal(false) + done() + } + ) + }) + }) + }) + describe('_makeGoogleCloudRequest', function() { beforeEach(function() { this.settings.apis.clsi_new = { url: 'https://compiles.somewhere.test' } diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index 929d4df20d..95529d012f 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -649,8 +649,9 @@ describe('CompileController', function() { describe('deleteAuxFiles', function() { beforeEach(function() { - this.CompileManager.deleteAuxFiles = sinon.stub().callsArg(2) + this.CompileManager.deleteAuxFiles = sinon.stub().yields() this.req.params = { Project_id: this.project_id } + this.req.query = { clsiserverid: 'node-1' } this.res.sendStatus = sinon.stub() return this.CompileController.deleteAuxFiles( this.req, @@ -661,7 +662,7 @@ describe('CompileController', function() { it('should proxy to the CLSI', function() { return this.CompileManager.deleteAuxFiles - .calledWith(this.project_id) + .calledWith(this.project_id, this.user_id, 'node-1') .should.equal(true) }) @@ -714,8 +715,9 @@ describe('CompileController', function() { beforeEach(function() { this.CompileManager.wordCount = sinon .stub() - .callsArgWith(3, null, { content: 'body' }) + .yields(null, { content: 'body' }) this.req.params = { Project_id: this.project_id } + this.req.query = { clsiserverid: 'node-42' } this.res.send = sinon.stub() this.res.contentType = sinon.stub() return this.CompileController.wordCount(this.req, this.res, this.next) @@ -723,7 +725,7 @@ describe('CompileController', function() { it('should proxy to the CLSI', function() { return this.CompileManager.wordCount - .calledWith(this.project_id, this.user_id, false) + .calledWith(this.project_id, this.user_id, false, 'node-42') .should.equal(true) })