joyent/node-triton#255 creating fabric networks still broken

joyent/node-triton#257 "triton vlan create" should not assert on missing argument
Reviewed by: Dave Eddy <dave.eddy@joyent.com>
Reviewed by: Robert Mustacchi <rm@joyent.com>
Approved by: Cody Mello <cody.mello@joyent.com>

CHANGES.md

@@ -6,6 +6,12 @@ Known issues:
 
 ## not yet released
 
+## 6.2.0
+
+- [joyent/node-triton#255, joyent/node-triton#257] Improved the interface
+  and documentation of `triton network create` and `triton vlan create`.  In
+  particular, it is now possible to specify static routes and DNS resolvers.
+
 ## 6.1.2
 
 - [joyent/node-triton#249] Error when creating or deleting profiles when

lib/cloudapi2.js

@@ -504,7 +504,7 @@ CloudApi.prototype.UPDATE_NETWORK_IP_FIELDS = {
 // --- Fabric VLANs
 
 /**
- * Creates a network on a fabric (specifically: a fabric VLAN).
+ * Creates a network on a fabric VLAN.
  *
  * @param {Object} options object containing:
  *      - {Integer} vlan_id (required) VLAN's id, between 0-4095.
@@ -513,7 +513,8 @@ CloudApi.prototype.UPDATE_NETWORK_IP_FIELDS = {
  *      - {String} provision_start_ip (required) First assignable IP addr.
  *      - {String} provision_end_ip (required) Last assignable IP addr.
  *      - {String} gateway (optional) Gateway IP address.
- *      - {String} resolvers (optional) Static routes for hosts on network.
+ *      - {Array} resolvers (optional) DNS resolvers for hosts on network.
+ *      - {Object} routes (optional) Static routes for hosts on network.
  *      - {String} description (optional)
  *      - {Boolean} internet_nat (optional) Whether to provision an Internet
  *          NAT on the gateway address (default: true).
@@ -528,8 +529,8 @@ function createFabricNetwork(opts, cb) {
     assert.string(opts.provision_start_ip, 'opts.provision_start_ip');
     assert.string(opts.provision_end_ip, 'opts.provision_end_ip');
     assert.optionalString(opts.gateway, 'opts.gateway');
-    assert.optionalString(opts.resolvers, 'opts.resolvers');
+    assert.optionalArrayOfString(opts.resolvers, 'opts.resolvers');
-    assert.optionalString(opts.routes, 'opts.routes');
+    assert.optionalObject(opts.routes, 'opts.routes');
     assert.optionalBool(opts.internet_nat, 'opts.internet_nat');
 
     var data = common.objCopy(opts);

lib/do_network/do_create.js

@@ -18,8 +18,6 @@ var common = require('../common');
 var errors = require('../errors');
 
 
-var OPTIONAL_OPTS = ['description', 'gateway', 'resolvers', 'routes'];
-
 
 function do_create(subcmd, opts, args, cb) {
     assert.optionalString(opts.name, 'opts.name');
@@ -28,13 +26,15 @@ function do_create(subcmd, opts, args, cb) {
     assert.optionalString(opts.end_ip, 'opts.end_ip');
     assert.optionalString(opts.description, 'opts.description');
     assert.optionalString(opts.gateway, 'opts.gateway');
-    assert.optionalString(opts.resolvers, 'opts.resolvers');
+    assert.optionalArrayOfString(opts.resolver, 'opts.resolver');
-    assert.optionalString(opts.routes, 'opts.routes');
+    assert.optionalArrayOfString(opts.route, 'opts.route');
     assert.optionalBool(opts.no_nat, 'opts.no_nat');
     assert.optionalBool(opts.json, 'opts.json');
     assert.optionalBool(opts.help, 'opts.help');
     assert.func(cb, 'cb');
 
+    var i;
+
     if (opts.help) {
         this.do_help('help', {}, [subcmd], cb);
         return;
@@ -53,23 +53,74 @@ function do_create(subcmd, opts, args, cb) {
         return;
     }
 
+    if (!opts.subnet) {
+        cb(new errors.UsageError('must specify --subnet (-s) option'));
+        return;
+    }
+
+    if (!opts.name) {
+        cb(new errors.UsageError('must specify --name (-n) option'));
+        return;
+    }
+
+    if (!opts.start_ip) {
+        cb(new errors.UsageError('must specify --start-ip (-S) option'));
+        return;
+    }
+
+    if (!opts.end_ip) {
+        cb(new errors.UsageError('must specify --end-ip (-E) option'));
+        return;
+    }
+
     var createOpts = {
         vlan_id: vlanId,
-        name:    opts.name,
+        name: opts.name,
-        subnet:  opts.subnet,
+        subnet: opts.subnet,
         provision_start_ip: opts.start_ip,
-        provision_end_ip:   opts.end_ip
+        provision_end_ip: opts.end_ip,
+        resolvers: [],
+        routes: {}
     };
 
+    if (opts.resolver) {
+        for (i = 0; i < opts.resolver.length; i++) {
+            if (createOpts.resolvers.indexOf(opts.resolver[i]) === -1) {
+                createOpts.resolvers.push(opts.resolver[i]);
+            }
+        }
+    }
+
+    if (opts.route) {
+        for (i = 0; i < opts.route.length; i++) {
+            var m = opts.route[i].match(new RegExp('^([^=]+)=([^=]+)$'));
+
+            if (m === null) {
+                cb(new errors.UsageError('invalid route: ' + opts.route[i]));
+                return;
+            }
+
+            createOpts.routes[m[1]] = m[2];
+        }
+    }
+
     if (opts.no_nat) {
         createOpts.internet_nat = false;
     }
 
-    OPTIONAL_OPTS.forEach(function (attr) {
+    if (opts.gateway) {
-        if (opts[attr]) {
+        createOpts.gateway = opts.gateway;
-            createOpts[attr] = opts[attr];
+    } else {
+        if (!opts.no_nat) {
+            cb(new errors.UsageError('without a --gateway (-g), you must ' +
+              'specify --no-nat (-x)'));
+            return;
         }
-    });
+    }
+
+    if (opts.description) {
+        createOpts.description = opts.description;
+    }
 
     var cli = this.top;
 
@@ -106,9 +157,7 @@ do_create.options = [
         help: 'Show this help.'
     },
     {
-        names: ['json', 'j'],
+        group: 'Create options'
-        type: 'bool',
-        help: 'JSON stream output.'
     },
     {
         names: ['name', 'n'],
@@ -123,46 +172,67 @@ do_create.options = [
         help: 'Description of the NETWORK.'
     },
     {
-        names: ['subnet'],
+        group: ''
+    },
+    {
+        names: ['subnet', 's'],
         type: 'string',
         helpArg: 'SUBNET',
         help: 'A CIDR string describing the NETWORK.'
     },
     {
-        names: ['start_ip'],
+        names: ['start-ip', 'S', 'start_ip'],
         type: 'string',
         helpArg: 'START_IP',
         help: 'First assignable IP address on NETWORK.'
     },
     {
-        names: ['end_ip'],
+        names: ['end-ip', 'E', 'end_ip'],
         type: 'string',
         helpArg: 'END_IP',
         help: 'Last assignable IP address on NETWORK.'
     },
     {
-        names: ['gateway'],
+        group: ''
-        type: 'string',
-        helpArg: 'GATEWAY',
-        help: 'Gateway IP address.'
     },
     {
-        names: ['resolvers'],
+        names: ['gateway', 'g'],
         type: 'string',
-        helpArg: 'RESOLVERS',
+        helpArg: 'IP',
-        help: 'Resolver IP addresses.'
+        help: 'Default gateway IP address.'
     },
     {
-        names: ['routes'],
+        names: ['resolver', 'r'],
-        type: 'string',
+        type: 'arrayOfString',
-        helpArg: 'ROUTES',
+        helpArg: 'RESOLVER',
-        help: 'Static routes for hosts on NETWORK.'
+        help: 'DNS resolver IP address.  Specify multiple -r options for ' +
+            'multiple resolvers.'
     },
     {
-        names: ['no_nat'],
+        names: ['route', 'R'],
+        type: 'arrayOfString',
+        helpArg: 'SUBNET=IP',
+        help: [ 'Static route for network.  Each route must include the',
+            'subnet (IP address with CIDR prefix length) and the router',
+            'address.  Specify multiple -R options for multiple static',
+            'routes.' ].join(' ')
+    },
+    {
+        group: ''
+    },
+    {
+        names: ['no-nat', 'x', 'no_nat'],
         type: 'bool',
         helpArg: 'NO_NAT',
         help: 'Disable creation of an Internet NAT zone on GATEWAY.'
+    },
+    {
+        group: 'Other options'
+    },
+    {
+        names: ['json', 'j'],
+        type: 'bool',
+        help: 'JSON stream output.'
     }
 ];
 
@@ -175,13 +245,29 @@ do_create.help = [
     '',
     '{{options}}',
     '',
-    'Example:',
+    'Examples:',
-    '    triton network create -n accounting --subnet=192.168.0.0/24',
+    '    Create the "accounting" network on VLAN 1000:',
-    '           --start_ip=192.168.0.1 --end_ip=192.168.0.254 2'
+    '        triton network create -n accounting --subnet 192.168.0.0/24 \\',
+    '            --start-ip 192.168.0.1 --end-ip 192.168.0.254 --no-nat \\',
+    '            1000',
+    '',
+    '    Create the "eng" network on VLAN 1001 with a pair of static routes:',
+    '        triton network create -n eng -s 192.168.1.0/24 \\',
+    '            -S 192.168.1.1 -E 192.168.1.249 --no-nat \\',
+    '            --route 10.1.1.0/24=192.168.1.50 \\',
+    '            --route 10.1.2.0/24=192.168.1.100 \\',
+    '            1001',
+    '',
+    '    Create the "ops" network on VLAN 1002 with DNS resolvers and NAT:',
+    '        triton network create -n ops -s 192.168.2.0/24 \\',
+    '            -S 192.168.2.10 -E 192.168.2.249 \\',
+    '            --resolver 8.8.8.8 --resolver 8.4.4.4 \\',
+    '            --gateway 192.168.2.1 \\',
+    '            1002'
 ].join('\n');
 
 do_create.helpOpts = {
-    helpCol: 25
+    helpCol: 16
 };
 
 module.exports = do_create;

lib/do_vlan/do_create.js

@@ -5,13 +5,14 @@
  */
 
 /*
- * Copyright 2017 Joyent, Inc.
+ * Copyright (c) 2018, Joyent, Inc.
  *
  * `triton vlan create ...`
  */
 
 var assert = require('assert-plus');
 var format = require('util').format;
+var jsprim = require('jsprim');
 var vasync = require('vasync');
 
 var common = require('../common');
@@ -37,12 +38,22 @@ function do_create(subcmd, opts, args, cb) {
         return;
     }
 
-    var createOpts = {
+    var vlanId = jsprim.parseInteger(args[0], { allowSign: false });
-        vlan_id: +args[0]
+    if (typeof (vlanId) !== 'number') {
-    };
+        cb(new errors.UsageError('VLAN must be an integer'));
-    if (opts.name) {
+        return;
-        createOpts.name = opts.name;
     }
+
+    if (!opts.name) {
+        cb(new errors.UsageError('must provide a --name (-n)'));
+        return;
+    }
+
+    var createOpts = {
+        vlan_id: vlanId,
+        name: opts.name
+    };
+
     if (opts.description) {
         createOpts.description = opts.description;
     }
@@ -86,9 +97,7 @@ do_create.options = [
         help: 'Show this help.'
     },
     {
-        names: ['json', 'j'],
+        group: 'Create options'
-        type: 'bool',
-        help: 'JSON stream output.'
     },
     {
         names: ['name', 'n'],
@@ -101,6 +110,14 @@ do_create.options = [
         type: 'string',
         helpArg: 'DESC',
         help: 'Description of the VLAN.'
+    },
+    {
+        group: 'Other options'
+    },
+    {
+        names: ['json', 'j'],
+        type: 'bool',
+        help: 'JSON stream output.'
     }
 ];
 
@@ -117,7 +134,7 @@ do_create.help = [
 ].join('\n');
 
 do_create.helpOpts = {
-    helpCol: 25
+    helpCol: 16
 };
 
 module.exports = do_create;

package.json

@@ -1,7 +1,7 @@
 {
   "name": "triton",
   "description": "Joyent Triton CLI and client (https://www.joyent.com/triton)",
-  "version": "6.1.2",
+  "version": "6.2.0",
   "author": "Joyent (joyent.com)",
   "homepage": "https://github.com/joyent/node-triton",
   "dependencies": {