PUBAPI-1266: Add instance disable-firewall/enable-firewall to node-triton (CR changes)

- Don't need confirmation/--force for undoable commands
- Drop duration from output message after wait. I don't feel the time
  for this should ever be long enough that the number is interesting
  enough to get top billing.
- If "shouldn't get here", then it should be an assert.
- Use '_' instead of '__' for unused options.
- Drop the res.instId hack. Instead use a partial faux instance
  in place where other endpoints return a machine object.
- Add 'tritoninstance' bash completion.
- Group together with 'fwrules' in 'triton inst' help output.

lib/cloudapi2.js

@@ -941,16 +941,16 @@ CloudApi.prototype.machineAudit = function machineAudit(id, cb) {
 
 
 /**
- * Wait for a machine's firewall to enable or disable.
+ * Wait for a machine's `firewall_enabled` field to go true/false.
  *
  * @param {Object} options
- *      - {String} id {required} machine id
+ *      - {String} id: Required. The machine UUID.
- *      - {Boolean} state {require} - desired state
+ *      - {Boolean} state: Required. The desired `firewall_enabled` state.
- *      - {Number} interval (optional) - time in ms to poll
+ *      - {Number} interval: Optional. Time (in ms) to poll.
  * @param {Function} callback of the form f(err, machine, res).
  */
-CloudApi.prototype.waitForMachineFirewallState =
+CloudApi.prototype.waitForMachineFirewallEnabled =
-function waitForMachineFirewallState(opts, cb) {
+function waitForMachineFirewallEnabled(opts, cb) {
     var self = this;
 
     assert.object(opts, 'opts');

lib/do_instance/do_disable_firewall.js

@@ -14,7 +14,6 @@ var assert = require('assert-plus');
 var format = require('util').format;
 var vasync = require('vasync');
 
-var common = require('../common');
 var errors = require('../errors');
 
 
@@ -32,88 +31,46 @@ function do_disable_firewall(subcmd, opts, args, cb) {
     }
 
     var cli = this.top;
-    var insts = args;
 
-    function wait(instId, startTime, next) {
+    function wait(name, id, next) {
-        var cloudapi = cli.tritonapi.cloudapi;
+        cli.tritonapi.cloudapi.waitForMachineFirewallEnabled({
-        var waiter = cloudapi.waitForMachineFirewallState.bind(cloudapi);
+            id: id,
-
-        waiter({
-            id: instId,
             state: false
         }, function (err, inst) {
             if (err) {
-                return next(err);
+                next(err);
+                return;
             }
+            assert.ok(!inst.firewall_enabled, format(
+                'inst %s firewall_enabled not in expected state after '
+                + 'waitForMachineFirewallEnabled', id));
 
-            if (inst.firewall_enabled === false) {
+            console.log('Disabled firewall for instance "%s"', name);
-                var duration = Date.now() - startTime;
+            next();
-                var durStr = common.humanDurationFromMs(duration);
-                console.log('Disabled firewall for instance "%s" in %s', instId,
-                            durStr);
-                next();
-            } else {
-                // shouldn't get here, but...
-                var msg = 'Failed to disable firewall for instance "%s"';
-                next(new Error(format(msg, instId)));
-            }
         });
     }
 
-    vasync.pipeline({funcs: [
+    vasync.forEachParallel({
-        function confirm(_, next) {
+        inputs: args,
-            if (opts.force) {
+        func: function disableOne(name, nextInst) {
-                return next();
+            cli.tritonapi.disableInstanceFirewall({
-            }
+                id: name
+            }, function (err, fauxInst) {
+                if (err) {
+                    nextInst(err);
+                    return;
+                }
 
-            var msg;
+                console.log('Disabling firewall for instance "%s"', name);
-            if (insts.length === 1) {
-                msg = 'Disable firewall for instance "' + insts[0] +
-                      '"? [y/n] ';
-            } else {
-                msg = format('Disable firewalls for %d instances (%s)? [y/n] ',
-                    insts.length, insts.join(', '));
-            }
 
-            common.promptYesNo({msg: msg}, function (answer) {
+                if (opts.wait) {
-                if (answer !== 'y') {
+                    wait(name, fauxInst.id, nextInst);
-                    console.error('Aborting');
-                    next(true); // early abort signal
                 } else {
-                    next();
+                    nextInst();
                 }
             });
-        },
-        function disableThem(_, next) {
-            var startTime = Date.now();
-
-            vasync.forEachParallel({
-                inputs: insts,
-                func: function disableOne(instId, nextId) {
-                    cli.tritonapi.disableInstanceFirewall({
-                        id: instId
-                    }, function (err, __, res) {
-                        if (err) {
-                            nextId(err);
-                            return;
-                        }
-
-                        var msg = 'Disabling firewall for instance "%s"';
-                        console.log(msg, res.instId);
-
-                        if (opts.wait) {
-                            wait(res.instId, startTime, nextId);
-                        } else {
-                            nextId();
-                        }
-                    });
-                }
-            }, next);
-        }
-    ]}, function (err) {
-        if (err === true) {
-            err = null;
         }
+    }, function (err) {
         cb(err);
     });
 }
@@ -125,11 +82,6 @@ do_disable_firewall.options = [
         type: 'bool',
         help: 'Show this help.'
     },
-    {
-        names: ['force', 'f'],
-        type: 'bool',
-        help: 'Skip confirmation to enable.'
-    },
     {
         names: ['wait', 'w'],
         type: 'bool',
@@ -145,4 +97,6 @@ do_disable_firewall.help = [
     '{{options}}'
 ].join('\n');
 
+do_disable_firewall.completionArgtypes = ['tritoninstance'];
+
 module.exports = do_disable_firewall;

lib/do_instance/do_enable_firewall.js

@@ -14,7 +14,6 @@ var assert = require('assert-plus');
 var format = require('util').format;
 var vasync = require('vasync');
 
-var common = require('../common');
 var errors = require('../errors');
 
 
@@ -32,87 +31,46 @@ function do_enable_firewall(subcmd, opts, args, cb) {
     }
 
     var cli = this.top;
-    var insts = args;
 
-    function wait(instId, startTime, next) {
+    function wait(name, id, next) {
-        var cloudapi = cli.tritonapi.cloudapi;
+        cli.tritonapi.cloudapi.waitForMachineFirewallEnabled({
-        var waiter = cloudapi.waitForMachineFirewallState.bind(cloudapi);
+            id: id,
-
-        waiter({
-            id: instId,
             state: true
         }, function (err, inst) {
             if (err) {
-                return next(err);
+                next(err);
+                return;
             }
+            assert.ok(inst.firewall_enabled, format(
+                'inst %s firewall_enabled not in expected state after '
+                + 'waitForMachineFirewallEnabled', id));
 
-            if (inst.firewall_enabled === true) {
+            console.log('Enabled firewall for instance "%s"', name);
-                var duration = Date.now() - startTime;
+            next();
-                var durStr = common.humanDurationFromMs(duration);
-                console.log('Enabled firewall for instance "%s" in %s', instId,
-                            durStr);
-                next();
-            } else {
-                // shouldn't get here, but...
-                var msg = 'Failed to enable firewall for instance "%s"';
-                next(new Error(format(msg, instId)));
-            }
         });
     }
 
-    vasync.pipeline({funcs: [
+    vasync.forEachParallel({
-        function confirm(_, next) {
+        inputs: args,
-            if (opts.force) {
+        func: function enableOne(name, nextInst) {
-                return next();
+            cli.tritonapi.enableInstanceFirewall({
-            }
+                id: name
+            }, function (err, fauxInst) {
+                if (err) {
+                    nextInst(err);
+                    return;
+                }
 
-            var msg;
+                console.log('Enabling firewall for instance "%s"', name);
-            if (insts.length === 1) {
-                msg = 'Enable firewall for instance "' + insts[0] + '"? [y/n] ';
-            } else {
-                msg = format('Enable firewalls for %d instances (%s)? [y/n] ',
-                    insts.length, insts.join(', '));
-            }
 
-            common.promptYesNo({msg: msg}, function (answer) {
+                if (opts.wait) {
-                if (answer !== 'y') {
+                    wait(name, fauxInst.id, nextInst);
-                    console.error('Aborting');
-                    next(true); // early abort signal
                 } else {
-                    next();
+                    nextInst();
                 }
             });
-        },
-        function enableThem(_, next) {
-            var startTime = Date.now();
-
-            vasync.forEachParallel({
-                inputs: insts,
-                func: function enableOne(instId, nextId) {
-                    cli.tritonapi.enableInstanceFirewall({
-                        id: instId
-                    }, function (err, __, res) {
-                        if (err) {
-                            nextId(err);
-                            return;
-                        }
-
-                        var msg = 'Enabling firewall for instance "%s"';
-                        console.log(msg, res.instId);
-
-                        if (opts.wait) {
-                            wait(res.instId, startTime, nextId);
-                        } else {
-                            nextId();
-                        }
-                    });
-                }
-            }, next);
-        }
-    ]}, function (err) {
-        if (err === true) {
-            err = null;
         }
+    }, function (err) {
         cb(err);
     });
 }
@@ -124,11 +82,6 @@ do_enable_firewall.options = [
         type: 'bool',
         help: 'Show this help.'
     },
-    {
-        names: ['force', 'f'],
-        type: 'bool',
-        help: 'Skip confirmation to enable.'
-    },
     {
         names: ['wait', 'w'],
         type: 'bool',
@@ -144,4 +97,6 @@ do_enable_firewall.help = [
     '{{options}}'
 ].join('\n');
 
+do_enable_firewall.completionArgtypes = ['tritoninstance'];
+
 module.exports = do_enable_firewall;

lib/do_instance/index.js

@@ -39,13 +39,13 @@ function InstanceCLI(top) {
             'stop',
             'reboot',
             { group: '' },
+            'fwrules',
             'enable-firewall',
             'disable-firewall',
             { group: '' },
             'ssh',
             'wait',
             'audit',
-            'fwrules',
             'snapshot',
             'tag'
         ]
@@ -67,13 +67,13 @@ InstanceCLI.prototype.do_start = require('./do_start');
 InstanceCLI.prototype.do_stop = require('./do_stop');
 InstanceCLI.prototype.do_reboot = require('./do_reboot');
 
+InstanceCLI.prototype.do_fwrules = require('./do_fwrules');
 InstanceCLI.prototype.do_enable_firewall = require('./do_enable_firewall');
 InstanceCLI.prototype.do_disable_firewall = require('./do_disable_firewall');
 
 InstanceCLI.prototype.do_ssh = require('./do_ssh');
 InstanceCLI.prototype.do_wait = require('./do_wait');
 InstanceCLI.prototype.do_audit = require('./do_audit');
-InstanceCLI.prototype.do_fwrules = require('./do_fwrules');
 InstanceCLI.prototype.do_snapshot = require('./do_snapshot');
 InstanceCLI.prototype.do_snapshots = require('./do_snapshots');
 InstanceCLI.prototype.do_tag = require('./do_tag');

lib/tritonapi.js

@@ -769,14 +769,20 @@ TritonApi.prototype.getInstance = function getInstance(opts, cb) {
 };
 
 
-// ---- instance firewall
+// ---- instance enable/disable firewall
 
 /**
  * Enable the firewall on an instance.
  *
  * @param {Object} opts
- *      - {String} id: The instance ID, or short ID. Required.
+ *      - {String} id: Required. The instance ID, name, or short ID.
- * @param {Function} callback `function (err, null, res)`
+ * @param {Function} callback `function (err, fauxInst, res)`
+ *      On failure `err` is an error instance, else it is null.
+ *      On success: `fauxInst` is an object with just the instance id,
+ *      `{id: <instance UUID>}` and `res` is the CloudAPI
+ *      `EnableMachineFirewall` response.
+ *      The API call does not return the instance/machine object, hence we
+ *      are limited to just the id for `fauxInst`.
  */
 TritonApi.prototype.enableInstanceFirewall =
 function enableInstanceFirewall(opts, cb) {
@@ -785,20 +791,22 @@ function enableInstanceFirewall(opts, cb) {
 
     var self = this;
     var res;
+    var fauxInst;
 
     vasync.pipeline({arg: {client: self, id: opts.id}, funcs: [
         _stepInstId,
 
         function enableFirewall(arg, next) {
+            fauxInst = {id: arg.instId};
+
             self.cloudapi.enableMachineFirewall(arg.instId,
-            function (err, _, _res) {
+                    function (err, _, _res) {
                 res = _res;
-                res.instId = arg.instId; // gross hack, in case caller needs it
                 next(err);
             });
         }
     ]}, function (err) {
-        cb(err, null, res);
+        cb(err, fauxInst, res);
     });
 };
 
@@ -807,8 +815,14 @@ function enableInstanceFirewall(opts, cb) {
  * Disable the firewall on an instance.
  *
  * @param {Object} opts
- *      - {String} id: The instance ID, or short ID. Required.
+ *      - {String} id: Required. The instance ID, name, or short ID.
- * @param {Function} callback `function (err, null, res)`
+ * @param {Function} callback `function (err, fauxInst, res)`
+ *      On failure `err` is an error instance, else it is null.
+ *      On success: `fauxInst` is an object with just the instance id,
+ *      `{id: <instance UUID>}` and `res` is the CloudAPI
+ *      `EnableMachineFirewall` response.
+ *      The API call does not return the instance/machine object, hence we
+ *      are limited to just the id for `fauxInst`.
  */
 TritonApi.prototype.disableInstanceFirewall =
 function disableInstanceFirewall(opts, cb) {
@@ -817,20 +831,22 @@ function disableInstanceFirewall(opts, cb) {
 
     var self = this;
     var res;
+    var fauxInst;
 
     vasync.pipeline({arg: {client: self, id: opts.id}, funcs: [
         _stepInstId,
 
         function disableFirewall(arg, next) {
+            fauxInst = {id: arg.instId};
+
             self.cloudapi.disableMachineFirewall(arg.instId,
-            function (err, _, _res) {
+                    function (err, _, _res) {
                 res = _res;
-                res.instId = arg.instId; // gross hack, in case caller needs it
                 next(err);
             });
         }
     ]}, function (err) {
-        cb(err, null, res);
+        cb(err, fauxInst, res);
     });
 };
 

test/integration/cli-fwrules.test.js

@@ -249,7 +249,7 @@ test('triton fwrule', OPTS, function (tt) {
     });
 
     tt.test('  triton instance enable-firewall', function (t) {
-        var cmd = 'instance enable-firewall ' + INST + ' -fw';
+        var cmd = 'instance enable-firewall ' + INST + ' -w';
         h.triton(cmd, function (err, stdout, stderr) {
             if (h.ifErr(t, err, 'triton instance enable-firewall'))
                 return t.end();
@@ -270,7 +270,7 @@ test('triton fwrule', OPTS, function (tt) {
     });
 
     tt.test('  triton instance disable-firewall', function (t) {
-        var cmd = 'instance disable-firewall ' + INST + ' -fw';
+        var cmd = 'instance disable-firewall ' + INST + ' -w';
         h.triton(cmd, function (err, stdout, stderr) {
             if (h.ifErr(t, err, 'triton instance disable-firewall'))
                 return t.end();