From 053d7354f24c3431ccda8b694502285678804b26 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 1 Mar 2016 16:46:06 -0800 Subject: [PATCH] PUBAPI-1233 Add firewalls to node-triton PUBAPI-1234 Add snapshots to node-triton CR updates. - add 'triton fwrule update ID ' completion on updatable fields - make ID arg first in ^^ - make 'triton fwrule create ...' have the rule enabled by default - cosmetic help output tweaks - allow 'triton fwrule update ...' to not *require* that the rule is updated. i.e you can update just the description --- etc/triton-bash-completion-types.sh | 7 +++ lib/cloudapi2.js | 18 ++++--- lib/do_completion.js | 9 ++-- lib/do_fwrule/do_create.js | 36 ++++++++----- lib/do_fwrule/do_update.js | 21 ++++---- lib/do_fwrule/index.js | 10 ++-- lib/do_image/index.js | 2 +- lib/do_instance/do_snapshot/do_create.js | 3 +- lib/do_instance/do_snapshot/index.js | 2 +- lib/do_instance/index.js | 2 +- lib/do_network/index.js | 2 +- lib/tritonapi.js | 69 ++++++++++++++++++------ test/integration/cli-fwrules.test.js | 47 +++++++++++++--- test/integration/cli-snapshots.test.js | 2 +- 14 files changed, 164 insertions(+), 66 deletions(-) diff --git a/etc/triton-bash-completion-types.sh b/etc/triton-bash-completion-types.sh index 2ae1e7f..4cc99fd 100644 --- a/etc/triton-bash-completion-types.sh +++ b/etc/triton-bash-completion-types.sh @@ -12,4 +12,11 @@ function complete_tritonupdateaccountfield { local candidates candidates="{{UPDATE_ACCOUNT_FIELDS}}" compgen $compgen_opts -W "$candidates" -- "$word" +} + +function complete_tritonupdatefwrulefield { + local word="$1" + local candidates + candidates="{{UPDATE_FWRULE_FIELDS}}" + compgen $compgen_opts -W "$candidates" -- "$word" } \ No newline at end of file diff --git a/lib/cloudapi2.js b/lib/cloudapi2.js index 51d54df..b722c6d 100644 --- a/lib/cloudapi2.js +++ b/lib/cloudapi2.js @@ -1195,7 +1195,7 @@ function getMachineSnapshot(opts, cb) { assert.func(cb, 'cb'); var endpoint = format('/%s/machines/%s/snapshots/%s', this.account, opts.id, - encodeURIComponent(opts.name)); + encodeURIComponent(opts.name)); this._passThrough(endpoint, opts, cb); }; @@ -1218,7 +1218,7 @@ function startMachineFromSnapshot(opts, cb) { this._request({ method: 'POST', path: format('/%s/machines/%s/snapshots/%s', this.account, opts.id, - encodeURIComponent(opts.name)), + encodeURIComponent(opts.name)), data: opts }, function (err, req, res, body) { cb(err, body, res); @@ -1244,7 +1244,7 @@ function deleteMachineSnapshot(opts, cb) { this._request({ method: 'DELETE', path: format('/%s/machines/%s/snapshots/%s', this.account, opts.id, - opts.name) + encodeURIComponent(opts.name)) }, function (err, req, res) { cb(err, res); }); @@ -1259,6 +1259,7 @@ function deleteMachineSnapshot(opts, cb) { * @param {Object} options object containing: * - {String} rule (required) the fwrule text. * - {Boolean} enabled (optional) default to false. + * - {String} description (optional) * @param {Function} callback of the form f(err, fwrule, res). */ CloudApi.prototype.createFirewallRule = @@ -1269,7 +1270,7 @@ function createFirewallRule(opts, cb) { assert.optionalBool(opts.enabled, 'opts.enabled'); var data = {}; - Object.keys(this.UPDATE_FIREWALL_RULE_FIELDS).forEach(function (attr) { + Object.keys(this.UPDATE_FWRULE_FIELDS).forEach(function (attr) { if (opts[attr] !== undefined) data[attr] = opts[attr]; }); @@ -1320,7 +1321,7 @@ function getFirewallRule(id, cb) { // -> -CloudApi.prototype.UPDATE_FIREWALL_RULE_FIELDS = { +CloudApi.prototype.UPDATE_FWRULE_FIELDS = { enabled: 'boolean', rule: 'string', description: 'string' @@ -1330,10 +1331,13 @@ CloudApi.prototype.UPDATE_FIREWALL_RULE_FIELDS = { /** * Updates a Firewall Rule. * + * Dev Note: That 'rule' is *required* here is lame. Hoping to change that + * in cloudapi. + * * @param {Object} opts object containing: * - {UUID} id: The fwrule id. Required. * - {String} rule: The fwrule text. Required. - * - {Boolean} enabled: Default to false. Optional. + * - {Boolean} enabled: Optional. * - {String} description: Description of the rule. Optional. * @param {Function} callback of the form `function (err, fwrule, res)` */ @@ -1347,7 +1351,7 @@ function updateFirewallRule(opts, cb) { assert.func(cb, 'cb'); var data = {}; - Object.keys(this.UPDATE_FIREWALL_RULE_FIELDS).forEach(function (attr) { + Object.keys(this.UPDATE_FWRULE_FIELDS).forEach(function (attr) { if (opts[attr] !== undefined) data[attr] = opts[attr]; }); diff --git a/lib/do_completion.js b/lib/do_completion.js index f6426da..9655292 100644 --- a/lib/do_completion.js +++ b/lib/do_completion.js @@ -13,8 +13,9 @@ var fs = require('fs'); var path = require('path'); -var UPDATE_ACCOUNT_FIELDS - = require('./cloudapi2').CloudApi.prototype.UPDATE_ACCOUNT_FIELDS; +var CloudApi = require('./cloudapi2').CloudApi; +var UPDATE_ACCOUNT_FIELDS = CloudApi.prototype.UPDATE_ACCOUNT_FIELDS; +var UPDATE_FWRULE_FIELDS = CloudApi.prototype.UPDATE_FWRULE_FIELDS; // Replace {{variable}} in `s` with the template data in `d`. @@ -39,6 +40,8 @@ function do_completion(subcmd, opts, args, cb) { 'utf8'); var specExtra = renderTemplate(specExtraIn, { UPDATE_ACCOUNT_FIELDS: Object.keys(UPDATE_ACCOUNT_FIELDS).sort() + .map(function (field) { return field + '='; }).join(' '), + UPDATE_FWRULE_FIELDS: Object.keys(UPDATE_FWRULE_FIELDS).sort() .map(function (field) { return field + '='; }).join(' ') }); console.log(this.bashCompletion({specExtra: specExtra})); @@ -61,7 +64,7 @@ do_completion.options = [ } ]; do_completion.help = [ - 'Output bash completion. See help output for installation.', + 'Emit bash completion. See help for installation.', '', 'Installation:', ' {{name}} completion > /usr/local/etc/bash_completion.d/{{name}} # Mac', diff --git a/lib/do_fwrule/do_create.js b/lib/do_fwrule/do_create.js index 1fb22af..cc24858 100644 --- a/lib/do_fwrule/do_create.js +++ b/lib/do_fwrule/do_create.js @@ -20,14 +20,13 @@ var errors = require('../errors'); function do_create(subcmd, opts, args, cb) { assert.optionalString(opts.description, 'opts.description'); - assert.optionalBool(opts.enabled, 'opts.enabled'); + assert.optionalBool(opts.disabled, 'opts.disabled'); assert.func(cb, 'cb'); if (opts.help) { this.do_help('help', {}, [subcmd], cb); return; } - if (args.length === 0) { cb(new errors.UsageError('missing argument')); return; @@ -36,17 +35,24 @@ function do_create(subcmd, opts, args, cb) { return; } - opts.rule = args[0]; + var createOpts = { + rule: args[0] + }; + if (!opts.disabled) { + createOpts.enabled = true; + } + if (opts.description) { + createOpts.description = opts.description; + } - var cli = this.top; - cli.tritonapi.cloudapi.createFirewallRule(opts, function (err, fwrule) { + this.top.tritonapi.cloudapi.createFirewallRule(createOpts, + function (err, fwrule) { if (err) { cb(err); return; } - - console.log('Created firewall rule %s', fwrule.id); - + console.log('Created firewall rule %s%s', fwrule.id, + (!fwrule.enabled ? ' (disabled)' : '')); cb(); }); } @@ -64,14 +70,16 @@ do_create.options = [ help: 'JSON stream output.' }, { - names: ['enabled', 'e'], + names: ['disabled', 'd'], type: 'bool', - help: 'If the firewall rule should be enabled upon creation.' + help: 'Disable the created firewall rule. By default a created ' + + 'firewall rule is enabled. Use "triton fwrule enable" ' + + 'to enable it later.' }, { - names: ['description', 'd'], + names: ['description', 'D'], type: 'string', - helpArg: '', + helpArg: '', help: 'Description of the firewall rule.' } ]; @@ -84,4 +92,8 @@ do_create.help = [ '{{options}}' ].join('\n'); +do_create.helpOpts = { + helpCol: 25 +}; + module.exports = do_create; diff --git a/lib/do_fwrule/do_update.js b/lib/do_fwrule/do_update.js index 05b4bac..e9c62e0 100644 --- a/lib/do_fwrule/do_update.js +++ b/lib/do_fwrule/do_update.js @@ -17,8 +17,8 @@ var vasync = require('vasync'); var common = require('../common'); var errors = require('../errors'); -var UPDATE_FIREWALL_RULE_FIELDS - = require('../cloudapi2').CloudApi.prototype.UPDATE_FIREWALL_RULE_FIELDS; +var UPDATE_FWRULE_FIELDS + = require('../cloudapi2').CloudApi.prototype.UPDATE_FWRULE_FIELDS; function do_update(subcmd, opts, args, cb) { @@ -35,7 +35,7 @@ function do_update(subcmd, opts, args, cb) { return; } - var id = args.pop(); + var id = args.shift(); vasync.pipeline({arg: {}, funcs: [ function gatherDataArgs(ctx, next) { @@ -47,7 +47,7 @@ function do_update(subcmd, opts, args, cb) { try { ctx.data = common.objFromKeyValueArgs(args, { disableDotted: true, - typeHintFromKey: UPDATE_FIREWALL_RULE_FIELDS + typeHintFromKey: UPDATE_FWRULE_FIELDS }); } catch (err) { next(err); @@ -116,13 +116,12 @@ function do_update(subcmd, opts, args, cb) { for (var i = 0; i < keys.length; i++) { var key = keys[i]; var value = ctx.data[key]; - var type = UPDATE_FIREWALL_RULE_FIELDS[key]; + var type = UPDATE_FWRULE_FIELDS[key]; if (!type) { next(new errors.UsageError(format('unknown or ' + 'unupdateable field: %s (updateable fields are: %s)', key, - Object.keys(UPDATE_FIREWALL_RULE_FIELDS).sort().join( - ', ')))); + Object.keys(UPDATE_FWRULE_FIELDS).sort().join(', ')))); return; } @@ -174,18 +173,18 @@ do_update.help = [ 'Update a firewall rule', '', 'Usage:', - ' {{name}} update [FIELD=VALUE ...] ', + ' {{name}} update [FIELD=VALUE ...]', ' {{name}} update -f ', '', '{{options}}', 'Updateable fields:', - ' ' + Object.keys(UPDATE_FIREWALL_RULE_FIELDS).sort().map(function (f) { - return f + ' (' + UPDATE_FIREWALL_RULE_FIELDS[f] + ')'; + ' ' + Object.keys(UPDATE_FWRULE_FIELDS).sort().map(function (f) { + return f + ' (' + UPDATE_FWRULE_FIELDS[f] + ')'; }).join('\n '), '' ].join('\n'); -do_update.completionArgtypes = ['tritonupdatefwrulefield']; +do_update.completionArgtypes = ['tritonfwrule', 'tritonupdatefwrulefield']; module.exports = do_update; diff --git a/lib/do_fwrule/index.js b/lib/do_fwrule/index.js index 8519a43..2b722d9 100644 --- a/lib/do_fwrule/index.js +++ b/lib/do_fwrule/index.js @@ -22,18 +22,22 @@ function FirewallRuleCLI(top) { Cmdln.call(this, { name: top.name + ' fwrule', - desc: 'List, get, create and update Triton firewall rules.', + desc: 'List and manage Triton firewall rules.', helpSubcmds: [ 'help', - 'create', 'list', 'get', + 'create', 'update', 'delete', + { group: '' }, 'enable', 'disable', 'instances' - ] + ], + helpOpts: { + minHelpCol: 23 + } }); } util.inherits(FirewallRuleCLI, Cmdln); diff --git a/lib/do_image/index.js b/lib/do_image/index.js index 40d0807..35fb759 100644 --- a/lib/do_image/index.js +++ b/lib/do_image/index.js @@ -23,7 +23,7 @@ function ImageCLI(top) { name: top.name + ' image', /* BEGIN JSSTYLED */ desc: [ - 'List, get, create and manage Triton images.' + 'List and manage Triton images.' ].join('\n'), /* END JSSTYLED */ helpOpts: { diff --git a/lib/do_instance/do_snapshot/do_create.js b/lib/do_instance/do_snapshot/do_create.js index ac33182..469e6b9 100644 --- a/lib/do_instance/do_snapshot/do_create.js +++ b/lib/do_instance/do_snapshot/do_create.js @@ -59,7 +59,8 @@ function do_create(subcmd, opts, args, cb) { return; } - console.log('Creating snapshot %s', snapshot.name); + console.log('Creating snapshot %s of instance %s', + snapshot.name, createOpts.id); ctx.name = snapshot.name; ctx.instId = res.instId; diff --git a/lib/do_instance/do_snapshot/index.js b/lib/do_instance/do_snapshot/index.js index 58d13f8..471dc6f 100644 --- a/lib/do_instance/do_snapshot/index.js +++ b/lib/do_instance/do_snapshot/index.js @@ -31,7 +31,7 @@ function SnapshotCLI(top) { 'delete' ], helpBody: 'Instances can be rolled back to a snapshot using\n' + - '`triton instance start --snapshot=`' + '`triton instance start --snapshot=`.' }); } util.inherits(SnapshotCLI, Cmdln); diff --git a/lib/do_instance/index.js b/lib/do_instance/index.js index 412ad6d..0484ebe 100644 --- a/lib/do_instance/index.js +++ b/lib/do_instance/index.js @@ -22,7 +22,7 @@ function InstanceCLI(top) { name: top.name + ' instance', /* BEGIN JSSTYLED */ desc: [ - 'List, get, create and manage Triton instances.' + 'List and manage Triton instances.' ].join('\n'), /* END JSSTYLED */ helpOpts: { diff --git a/lib/do_network/index.js b/lib/do_network/index.js index 5d01012..e22c576 100644 --- a/lib/do_network/index.js +++ b/lib/do_network/index.js @@ -23,7 +23,7 @@ function NetworkCLI(top) { name: top.name + ' network', /* BEGIN JSSTYLED */ desc: [ - 'List, get, create and update Triton networks.' + 'List and manage Triton networks.' ].join('\n'), /* END JSSTYLED */ helpOpts: { diff --git a/lib/tritonapi.js b/lib/tritonapi.js index 7827911..e338b42 100644 --- a/lib/tritonapi.js +++ b/lib/tritonapi.js @@ -88,8 +88,10 @@ function _stepInstId(arg, next) { /** * A function appropriate for `vasync.pipeline` funcs that takes a `arg.id` - * instance name, shortid or uuid, and determines the fwrule id (setting it + * fwrule shortid or uuid, and determines the fwrule id (setting it * as `arg.fwruleId`). + * + * If the fwrule *was* retrieved, that is set as `arg.fwrule`. */ function _stepFwRuleId(arg, next) { assert.object(arg.client, 'arg.client'); @@ -1496,10 +1498,10 @@ function listInstanceFirewallRules(opts, cb) { /** - * List all machines affected by a firewall rule. + * List all instances affected by a firewall rule. * * @param {Object} opts - * - {String} id: The fwrule ID, name, or short ID. Required. + * - {String} id: The fwrule ID, or short ID. Required. * @param {Function} callback `function (err, instances, res)` */ TritonApi.prototype.listFirewallRuleInstances = @@ -1514,7 +1516,7 @@ function listFirewallRuleInstances(opts, cb) { vasync.pipeline({arg: {client: self, id: opts.id}, funcs: [ _stepFwRuleId, - function listMachines(arg, next) { + function listInsts(arg, next) { self.cloudapi.listFirewallRuleMachines({ id: arg.fwruleId }, function (err, machines, _res) { @@ -1532,37 +1534,70 @@ function listFirewallRuleInstances(opts, cb) { /** * Update a firewall rule. * + * Dev Note: Currently cloudapi UpdateFirewallRule *requires* the 'rule' field, + * which is overkill. `TritonApi.updateFirewallRule` adds sugar by making + * 'rule' optional. + * * @param {Object} opts - * - {String} id: The fwrule ID, name, or short ID. Required. - * - {String} rule: The fwrule text. Required. + * - {String} id: The fwrule ID, or short ID. Required. + * - {String} rule: The fwrule text. Optional. * - {Boolean} enabled: Default to false. Optional. * - {String} description: Description of the rule. Optional. - * @param {Function} callback `function (err, instances, res)` + * At least one of the fields must be provided. + * @param {Function} callback `function (err, fwrule, res)` */ TritonApi.prototype.updateFirewallRule = function updateFirewallRule(opts, cb) { + // TODO: strict opts field validation assert.string(opts.id, 'opts.id'); - assert.string(opts.rule, 'opts.rule'); + assert.optionalString(opts.rule, 'opts.rule'); assert.optionalBool(opts.enabled, 'opts.enabled'); assert.optionalString(opts.description, 'opts.description'); + assert.ok(opts.rule !== undefined || opts.enabled !== undefined || + opts.description !== undefined, 'at least one of opts.rule, ' + + 'opts.enabled, or opts.description is required'); assert.func(cb, 'cb'); var self = this; var res; - var fwrule; + var updatedFwrule; + var updateOpts = common.objCopy(opts); vasync.pipeline({arg: {client: self, id: opts.id}, funcs: [ _stepFwRuleId, + /* + * CloudAPI currently requires the 'rule' field. We provide sugar here + * and fill it in for you. + */ + function sugarFillRuleField(arg, next) { + if (updateOpts.rule) { + next(); + } else if (arg.fwrule) { + updateOpts.rule = arg.fwrule.rule; + next(); + } else { + self.getFirewallRule(arg.fwruleId, function (err, fwrule) { + if (err) { + next(err); + } else { + updateOpts.rule = fwrule.rule; + next(); + } + }); + } + }, + function updateRule(arg, next) { - opts.id = arg.fwruleId; - self.cloudapi.updateFirewallRule(opts, function (err, rule, _res) { - res = _res; - fwrule = rule; + updateOpts.id = arg.fwruleId; + self.cloudapi.updateFirewallRule(updateOpts, + function (err, fwrule, res_) { + res = res_; + updatedFwrule = fwrule; next(err); }); } ]}, function (err) { - cb(err, fwrule, res); + cb(err, updatedFwrule, res); }); }; @@ -1571,7 +1606,7 @@ TritonApi.prototype.updateFirewallRule = function updateFirewallRule(opts, cb) { * Enable a firewall rule. * * @param {Object} opts - * - {String} id: The fwrule ID, name, or short ID. Required. + * - {String} id: The fwrule ID, or short ID. Required. * @param {Function} callback `function (err, fwrule, res)` */ TritonApi.prototype.enableFirewallRule = function enableFirewallRule(opts, cb) { @@ -1604,7 +1639,7 @@ TritonApi.prototype.enableFirewallRule = function enableFirewallRule(opts, cb) { * Disable a firewall rule. * * @param {Object} opts - * - {String} id: The fwrule ID, name, or short ID. Required. + * - {String} id: The fwrule ID, or short ID. Required. * @param {Function} callback `function (err, fwrule, res)` */ TritonApi.prototype.disableFirewallRule = @@ -1638,7 +1673,7 @@ function disableFirewallRule(opts, cb) { * Delete a firewall rule. * * @param {Object} opts - * - {String} id: The fwrule ID, name, or short ID. Required. + * - {String} id: The fwrule ID, or short ID. Required. * @param {Function} callback `function (err, res)` * */ diff --git a/test/integration/cli-fwrules.test.js b/test/integration/cli-fwrules.test.js index af2bd40..c1b2e0f 100644 --- a/test/integration/cli-fwrules.test.js +++ b/test/integration/cli-fwrules.test.js @@ -59,15 +59,49 @@ test('triton fwrule', OPTS, function (tt) { }); }); + tt.test(' triton fwrule create --disabled', function (t) { + var cmd = f('fwrule create -d "%s"', RULE); + h.triton(cmd, function (err, stdout, stderr) { + if (h.ifErr(t, err, 'triton fwrule create --disabled')) + return t.end(); + /* JSSTYLED */ + var expected = /^Created firewall rule ([a-f0-9-]{36}) \(disabled\)$/m; + var match = expected.exec(stdout); + t.ok(match, f('stdout matches %s: %j', expected, stdout)); + + var id = match[1]; + t.ok(id); + ID = id.match(/^(.+?)-/)[1]; // convert to short ID + + t.end(); + }); + }); + + tt.test(' triton fwrule get (disabled)', function (t) { + var cmd = 'fwrule get ' + ID; + + h.triton(cmd, function (err, stdout, stderr) { + if (h.ifErr(t, err, 'triton fwrule get')) + return t.end(); + + var obj = JSON.parse(stdout); + t.equal(obj.rule, RULE, 'fwrule rule is correct'); + t.equal(obj.enabled, false, 'fwrule is disabled'); + t.end(); + }); + }); + tt.test(' triton fwrule create', function (t) { - var cmd = f('fwrule create -d "%s" "%s"', DESC, RULE); + var cmd = f('fwrule create -D "%s" "%s"', DESC, RULE); h.triton(cmd, function (err, stdout, stderr) { if (h.ifErr(t, err, 'triton fwrule create')) return t.end(); - var match = stdout.match('Created firewall rule (.+)'); - t.ok(match, 'fwrule made'); + /* JSSTYLED */ + var expected = /^Created firewall rule ([a-f0-9-]{36})$/m; + var match = expected.exec(stdout); + t.ok(match, f('stdout matches %s: %j', expected, stdout)); var id = match[1]; t.ok(id); @@ -87,8 +121,7 @@ test('triton fwrule', OPTS, function (tt) { var obj = JSON.parse(stdout); t.equal(obj.rule, RULE, 'fwrule rule is correct'); t.equal(obj.description, DESC, 'fwrule was properly created'); - t.equal(obj.enabled, false, 'fwrule enabled defaults to false'); - + t.equal(obj.enabled, true, 'fwrule enabled defaults to true'); t.end(); }); }); @@ -120,10 +153,10 @@ test('triton fwrule', OPTS, function (tt) { }); tt.test(' triton fwrule update', function (t) { - var cmd = 'fwrule update rule="' + RULE2 + '" ' + ID; + var cmd = 'fwrule update ' + ID + ' rule="' + RULE2 + '"'; h.triton(cmd, function (err, stdout, stderr) { - if (h.ifErr(t, err, 'triton fwrule disable')) + if (h.ifErr(t, err, 'triton fwrule update')) return t.end(); t.ok(stdout.match('Updated firewall rule ' + ID + diff --git a/test/integration/cli-snapshots.test.js b/test/integration/cli-snapshots.test.js index 38f856e..47ed0e3 100644 --- a/test/integration/cli-snapshots.test.js +++ b/test/integration/cli-snapshots.test.js @@ -20,7 +20,7 @@ var test = require('tape'); // --- Globals var SNAP_NAME = 'test-snapshot'; -var INST_ALIAS = f('nodetritontest-fwrules-%s', os.hostname()); +var INST_ALIAS = f('nodetritontest-snapshots-%s', os.hostname()); var INST; var OPTS = { skip: !h.CONFIG.allowWriteActions