From ba5255416ad7fcf08334de2016a9d7b8a0b033b1 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 12 Dec 2016 11:51:37 -0800 Subject: [PATCH] switch to using @google-cloud/common MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR starts using the authorized / retry request flow from the @google-cloud/common library. From an operational perspective This shouldn't change behaviour significantly – although there are small differences in how the new library does retries or how it prioritizes different sources of the projectId & credentials (environment vs. metadata vs. config). --- .gitignore | 1 + package.json | 1 + src/agent/debuglet.js | 7 +++-- src/debugletapi.js | 35 ++++++++-------------- src/index.js | 28 +++++++++++++---- test/auth-request.js | 23 ++++++++++++++ test/e2e/test-breakpoints.js | 16 ++++------ test/e2e/test-log-throttling.js | 8 ++--- test/standalone/test-config-credentials.js | 32 +++++++++++--------- test/standalone/test-debuglet.js | 14 ++++++--- test/test-debugletapi.js | 11 +++---- 11 files changed, 109 insertions(+), 67 deletions(-) create mode 100644 test/auth-request.js diff --git a/.gitignore b/.gitignore index 83e1c349..261bde49 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ node_modules coverage npm-debug.log .DS_Store +.vscode diff --git a/package.json b/package.json index 72e4e134..404a22eb 100644 --- a/package.json +++ b/package.json @@ -32,6 +32,7 @@ "proxyquire": "^1.4.0" }, "dependencies": { + "@google-cloud/common": "^0.9.0", "@google/cloud-diagnostics-common": "0.3.0", "acorn": "^4.0.3", "async": "^2.1.2", diff --git a/src/agent/debuglet.js b/src/agent/debuglet.js index de6a8f01..e12a6e56 100644 --- a/src/agent/debuglet.js +++ b/src/agent/debuglet.js @@ -46,7 +46,10 @@ module.exports = Debuglet; * @event 'stopped' if the agent stops due to a fatal error after starting * @constructor */ -function Debuglet(config, logger) { +function Debuglet(debug, config, logger) { + /** @private {Debug} */ + this.debug_ = debug; + /** @private {object} */ this.config_ = config || {}; @@ -69,7 +72,7 @@ function Debuglet(config, logger) { this.logger_ = logger; /** @private {DebugletApi} */ - this.debugletApi_ = new DebugletApi(config); + this.debugletApi_ = new DebugletApi(this.config_, this.debug_); /** @private {Object.} */ this.activeBreakpointMap_ = {}; diff --git a/src/debugletapi.js b/src/debugletapi.js index b41fd201..e80508d8 100644 --- a/src/debugletapi.js +++ b/src/debugletapi.js @@ -33,23 +33,15 @@ var API = 'https://clouddebugger.googleapis.com/v2/controller'; /** @const {string} */ var DEBUGGEE_MAJOR_VERSION_LABEL = 'version'; /** @const {string} */ var DEBUGGEE_MINOR_VERSION_LABEL = 'minorversion'; -/** @const {Array} list of scopes needed to operate with the debug API */ -var SCOPES = [ - 'https://www.googleapis.com/auth/cloud-platform', - 'https://www.googleapis.com/auth/cloud_debugletcontroller' -]; /** * @constructor */ -function DebugletApi(config) { - var config_ = config || {}; +function DebugletApi(config, debug) { + config = config || {}; - /** @private {Object} request style request object */ - this.request_ = utils.authorizedRequestFactory(SCOPES, { - keyFile: config_.keyFilename, - credentials: config_.credentials - }); + /** @priavate {Debug} */ + this.debug_ = debug; /** @private {string} numeric project id */ this.project_ = null; @@ -58,13 +50,13 @@ function DebugletApi(config) { this.debuggeeId_ = null; /** @private {string} a descriptor of the current code version */ - this.descriptor_ = config_.description; + this.descriptor_ = config.description; /** @private {string} the service name of the current code */ - this.serviceName_ = config_.serviceContext && config_.serviceContext.service; + this.serviceName_ = config.serviceContext && config.serviceContext.service; /** @private {string} the version of the current code */ - this.serviceVersion_ = config_.serviceContext && config_.serviceContext.version; + this.serviceVersion_ = config.serviceContext && config.serviceContext.version; } /** @@ -196,13 +188,12 @@ DebugletApi.prototype.register_ = function(errorMessage, callback) { } var options = { - url: API + '/debuggees/register', + uri: API + '/debuggees/register', method: 'POST', json: true, body: { debuggee: debuggee } }; - - that.request_(options, function(err, response, body) { + this.debug_.request(options, function(err, body, response) { if (err) { callback(err); } else if (response.statusCode !== 200) { @@ -231,9 +222,9 @@ DebugletApi.prototype.listBreakpoints = function(callback) { query.waitToken = that.nextWaitToken; } - var url = API + '/debuggees/' + encodeURIComponent(that.debuggeeId_) + + var uri = API + '/debuggees/' + encodeURIComponent(that.debuggeeId_) + '/breakpoints?' + qs.stringify(query); - that.request_({url: url, json: true}, function(err, response, body) { + that.debug_.request({uri: uri, json: true}, function(err, body, response) { if (!response) { callback(err || new Error('unknown error - request response missing')); return; @@ -266,7 +257,7 @@ DebugletApi.prototype.updateBreakpoint = breakpoint.action = 'capture'; breakpoint.isFinalState = true; var options = { - url: API + '/debuggees/' + encodeURIComponent(this.debuggeeId_) + + uri: API + '/debuggees/' + encodeURIComponent(this.debuggeeId_) + '/breakpoints/' + encodeURIComponent(breakpoint.id), json: true, method: 'PUT', @@ -281,7 +272,7 @@ DebugletApi.prototype.updateBreakpoint = // stringify them. The try-catch keeps it resilient and avoids crashing the // user's app. try { - this.request_(options, function(err, response, body) { + this.debug_.request(options, function(err, body, response) { callback(err, body); }); } catch (error) { diff --git a/src/index.js b/src/index.js index fdeaa01e..59deac75 100644 --- a/src/index.js +++ b/src/index.js @@ -17,7 +17,9 @@ 'use strict'; var logger = require('@google/cloud-diagnostics-common').logger; +var common = require('@google-cloud/common'); var Debuglet = require('./agent/debuglet.js'); +var util = require('util'); var _ = require('lodash'); /** @@ -45,11 +47,24 @@ var _ = require('lodash'); */ function Debug(options) { if (!(this instanceof Debug)) { - //TODO(ofrobots) - //options = common.util.normalizeArguments(this, options); + options = common.util.normalizeArguments(this, options); return new Debug(options); } + + var config = { + baseUrl: 'https://clouddebugger.googleapis.com/v2', + scopes: [ + // TODO: do we still need cloud-platform scope? + 'https://www.googleapis.com/auth/cloud-platform', + 'https://www.googleapis.com/auth/cloud_debugletcontroller' + // TODO: the client library probably wants cloud_debugger scope as well. + ], + packageJson: require('../package.json') + }; + + common.Service.call(this, config, options); } +util.inherits(Debug, common.Service); var initConfig = function(config_) { var config = config_ || {}; @@ -77,7 +92,7 @@ var debuglet; * with Stackdriver Debug. * * @param {object=} config - Debug configuration. TODO(ofrobots): get rid of - * config.js and include jsdoc here + * config.js and include jsdoc here? * TODO: add an optional callback function. * * @resource [Introductory video]{@link https://www.youtube.com/watch?v=tyHcK_kAOpw} @@ -85,13 +100,14 @@ var debuglet; * @example * debug.startAgent(); */ -Debug.prototype.startAgent = function(config_) { +Debug.prototype.startAgent = function(config) { if (debuglet) { throw new Error('Debug Agent has already been started'); } - var config = initConfig(config_); + config = initConfig(config); if (config.enabled) { - debuglet = new Debuglet(config, logger.create(config.logLevel, '@google-cloud/debug')); + debuglet = new Debuglet( + this, config, logger.create(config.logLevel, '@google-cloud/debug')); debuglet.start(); this.private_ = debuglet; } diff --git a/test/auth-request.js b/test/auth-request.js new file mode 100644 index 00000000..1564c742 --- /dev/null +++ b/test/auth-request.js @@ -0,0 +1,23 @@ +/** + * Copyright 2016 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +'use strict'; + +var request = require('request'); +module.exports = function(options, callback) { + request(options, function(err, response, body) { + callback(err, body, response); + }); +}; diff --git a/test/e2e/test-breakpoints.js b/test/e2e/test-breakpoints.js index 1825078a..f0e64676 100644 --- a/test/e2e/test-breakpoints.js +++ b/test/e2e/test-breakpoints.js @@ -250,15 +250,15 @@ function runTest() { // and log points. if (cluster.isMaster) { cluster.setupMaster({ silent: true }); - var handler = function(m) { + var handler = function(a) { if (!debuggee) { // Cache the needed info from the first worker. - debuggee = m.private_.debugletApi_.debuggeeId_; - project = m.private_.debugletApi_.project_; + debuggee = a[0]; + project = a[1]; } else { // Make sure all other workers are consistent. - assert.equal(debuggee, m.private_.debugletApi_.debuggeeId_); - assert.equal(project, m.private_.debugletApi_.project_); + assert.equal(debuggee, a[0]); + assert.equal(project, a[1]); } }; var stdoutHandler = function(chunk) { @@ -288,12 +288,8 @@ if (cluster.isMaster) { assert.ok(api.uid_, 'debuglet provided unique id'); assert.ok(api.debuggeeId_, 'debuglet has registered'); // The parent process needs to know the debuggeeId and project. - process.send(debug); + process.send([api.debuggeeId_, api.project_]); setInterval(fib.bind(null, 12), 2000); }, 7000); } - -process.on('exit', function() { - console.log('worker transcript:', transcript); -}); diff --git a/test/e2e/test-log-throttling.js b/test/e2e/test-log-throttling.js index 6b6d92aa..80bee4f4 100644 --- a/test/e2e/test-log-throttling.js +++ b/test/e2e/test-log-throttling.js @@ -181,11 +181,11 @@ function runTest() { if (cluster.isMaster) { cluster.setupMaster({ silent: true }); - var handler = function(m) { + var handler = function(a) { // Cache the needed info from the first worker. if (!debuggee) { - debuggee = m.private_.debugletApi_.debuggeeId_; - project = m.private_.debugletApi_.project_; + debuggee = a[0]; + project = a[1]; } }; var stdoutHandler = function(chunk) { @@ -216,7 +216,7 @@ if (cluster.isMaster) { var api = debuglet.debugletApi_; assert.ok(api.uid_, 'debuglet provided unique id'); assert.ok(api.debuggeeId_, 'debuglet has registered'); - process.send(debug); + process.send([api.debuggeeId_, api.project_]); setInterval(fib.bind(null, 12), 500); }, 7000); } \ No newline at end of file diff --git a/test/standalone/test-config-credentials.js b/test/standalone/test-config-credentials.js index 60ab994c..0ebabb1c 100644 --- a/test/standalone/test-config-credentials.js +++ b/test/standalone/test-config-credentials.js @@ -45,6 +45,7 @@ describe('test-config-credentials', function() { var config = extend({}, defaultConfig, { keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json') }); + var debug = require('../..')(config); var scope = nock('https://accounts.google.com') .post('/o/oauth2/token', function(body) { assert.equal(body.client_id, credentials.client_id); @@ -63,7 +64,7 @@ describe('test-config-credentials', function() { setImmediate(done); return true; }).reply(200); - debuglet = new Debuglet(config, logger.create(logger.WARN, 'testing')); + debuglet = new Debuglet(debug, config, logger.create(logger.WARN, 'testing')); debuglet.start(); }); @@ -71,6 +72,7 @@ describe('test-config-credentials', function() { var config = extend({}, defaultConfig, { credentials: require('../fixtures/gcloud-credentials.json') }); + var debug = require('../..')(config); var scope = nock('https://accounts.google.com') .post('/o/oauth2/token', function(body) { assert.equal(body.client_id, config.credentials.client_id); @@ -89,32 +91,34 @@ describe('test-config-credentials', function() { setImmediate(done); return true; }).reply(200); - debuglet = new Debuglet(config, logger.create(undefined, 'testing')); + debuglet = new Debuglet(debug, config, logger.create(undefined, 'testing')); debuglet.start(); }); - it('should ignore credentials if keyFilename is provided', function(done) { - var correctCredentials = require('../fixtures/gcloud-credentials.json'); - var config = extend({}, defaultConfig, { - keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json'), - credentials: { + it('should ignore keyFilename if credentials is provided', function(done) { + var fileCredentials = require('../fixtures/gcloud-credentials.json'); + var credentials = { client_id: 'a', client_secret: 'b', refresh_token: 'c', type: 'authorized_user' - } + }; + var config = extend({}, defaultConfig, { + keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json'), + credentials: credentials }); + var debug = require('../..')(config); ['client_id', 'client_secret', 'refresh_token'].forEach(function (field) { - assert(correctCredentials.hasOwnProperty(field)); + assert(fileCredentials.hasOwnProperty(field)); assert(config.credentials.hasOwnProperty(field)); assert.notEqual(config.credentials[field], - correctCredentials[field]); + fileCredentials[field]); }); var scope = nock('https://accounts.google.com') .post('/o/oauth2/token', function(body) { - assert.equal(body.client_id, correctCredentials.client_id); - assert.equal(body.client_secret, correctCredentials.client_secret); - assert.equal(body.refresh_token, correctCredentials.refresh_token); + assert.equal(body.client_id, credentials.client_id); + assert.equal(body.client_secret, credentials.client_secret); + assert.equal(body.refresh_token, credentials.refresh_token); return true; }).reply(200, { refresh_token: 'hello', @@ -128,7 +132,7 @@ describe('test-config-credentials', function() { setImmediate(done); return true; }).reply(200); - debuglet = new Debuglet(config, logger.create(undefined, 'testing')); + debuglet = new Debuglet(debug, config, logger.create(undefined, 'testing')); debuglet.start(); }); }); diff --git a/test/standalone/test-debuglet.js b/test/standalone/test-debuglet.js index 1cff5917..f4f4bedb 100644 --- a/test/standalone/test-debuglet.js +++ b/test/standalone/test-debuglet.js @@ -16,7 +16,7 @@ 'use strict'; var assert = require('assert'); -var request = require('request'); +var request = require('../auth-request.js'); var logger = require('@google/cloud-diagnostics-common').logger; var config = require('../../src/config.js').debug; var Debuglet = require('../../src/agent/debuglet.js'); @@ -29,6 +29,9 @@ var BPS_PATH = '/v2/controller/debuggees/' + DEBUGGEE_ID + '/breakpoints'; var nock = require('nock'); nock.disableNetConnect(); +// Disable error logging during the tests. +config.logLevel = 0; + var debuglet; var bp = { id: 'test', @@ -40,12 +43,15 @@ var errorBp = { action: 'FOO', location: { path: 'fixtures/foo.js', line: 2 } }; +var fakeDebug = { + request: request +}; describe(__filename, function(){ beforeEach(function() { process.env.GCLOUD_PROJECT = 0; debuglet = new Debuglet( - config, logger.create(config.logLevel, '@google/cloud-debug')); + fakeDebug, config, logger.create(config.logLevel, '@google/cloud-debug')); debuglet.once('started', function() { debuglet.debugletApi_.request_ = request; // Avoid authing. }); @@ -236,7 +242,7 @@ describe(__filename, function(){ config.breakpointExpirationSec = 1; this.timeout(6000); var debuglet = new Debuglet( - config, logger.create(config.logLevel, '@google/cloud-debug')); + fakeDebug, config, logger.create(config.logLevel, '@google/cloud-debug')); var scope = nock(API) .post(REGISTER_PATH) @@ -288,7 +294,7 @@ describe(__filename, function(){ config.breakpointUpdateIntervalSec = 1; this.timeout(6000); var debuglet = new Debuglet( - config, logger.create(config.logLevel, '@google/cloud-debug')); + fakeDebug, config, logger.create(config.logLevel, '@google/cloud-debug')); var scope = nock(API) .post(REGISTER_PATH) diff --git a/test/test-debugletapi.js b/test/test-debugletapi.js index 30d41825..fe85435b 100644 --- a/test/test-debugletapi.js +++ b/test/test-debugletapi.js @@ -17,7 +17,7 @@ var assert = require('assert'); var nock = require('nock'); -var request = require('request'); +var request = require('./auth-request.js'); var proxyquire = require('proxyquire'); var agentVersion = require('../package.json').version; @@ -28,8 +28,6 @@ delete process.env.GCLOUD_PROJECT; // require DebugletAPI while stubbing auth to bypass authentication // var utils = { - // return vanilla request to bypass authentication - authorizedRequestFactory: function(/*scopes*/) { return request; }, getProjectNumber: function(callback) { callback(null, 'project123'); } }; var DebugletApi = proxyquire('../src/debugletapi.js', { @@ -38,6 +36,9 @@ var DebugletApi = proxyquire('../src/debugletapi.js', { utils: utils } }); +var fakeDebug = { + request: request +}; var url = 'https://clouddebugger.googleapis.com'; var api = '/v2/controller'; @@ -52,7 +53,7 @@ describe('Debuglet API', function() { service: 'TestDebugletName', version: 'TestDebugletVersion' } - }); + }, fakeDebug); // use vanilla request to bypass authentication it('should return an instance when constructed', function() { assert.ok(debugletapi); @@ -107,7 +108,7 @@ describe('Debuglet API', function() { callback(new Error(), null); }; process.env.GCLOUD_PROJECT = 'project123'; - var debugletapi = new DebugletApi(); + var debugletapi = new DebugletApi(null, fakeDebug); debugletapi.init('uid1234', { warn: function() {} }, function(err, project) { var scope = nock(url) .post(api + '/debuggees/register', function (body) {