Many node names now break the system

[DavidMarchant]

Due the node range expansion introduced in https://github.com/alces-software/adminware/commit/be4c96f7ab8f69bf1ada932b98cf63aca0bee891 many values for --node now result in an error & a crash. This is due to the change to nodes being built into a genders file and then parsed with the '--expand' genders option - any invalid nodename breaks this process. Any nodename that contains a period is invalid

``` adminware> batch run --node root192.168.33.10 test_cmd3 Traceback (most recent call last): File "src/adminware", line 7, in adminware() File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 722, in __call__ return self.main(*args, **kwargs) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 697, in main rv = self.invoke(ctx) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1066, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 895, in invoke return ctx.invoke(self.callback, **ctx.params) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 535, in invoke return callback(*args, **kwargs) File "/tmp/adminware/src/appliance_cli/sandbox.py", line 27, in sandbox allow_internal_commands=False, File "/tmp/adminware/venv/src/click-repl/click_repl/__init__.py", line 194, in repl group.invoke(ctx) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1066, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1066, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1063, in invoke Command.invoke(self, ctx) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 895, in invoke return ctx.invoke(self.callback, **ctx.params) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 535, in invoke return callback(*args, **kwargs) File "/tmp/adminware/venv/lib/python3.5/site-packages/click/decorators.py", line 17, in new_func return f(get_current_context(), *args, **kwargs) File "/tmp/adminware/src/commands/batch.py", line 110, in run set_nodes_context(ctx, **kwargs) File "/tmp/adminware/src/cli_utils.py", line 10, in set_nodes_context obj['nodes'].extend(groups.expand_nodes(kwargs['node'])) File "/tmp/adminware/src/groups.py", line 20, in expand_nodes nodes = __nodeattr(file_path=tmp_file.name)('--expand').split("\n") File "/tmp/adminware/venv/lib/python3.5/site-packages/plumbum/commands/base.py", line 103, in __call__ return self.run(args, **kwargs)[1] File "/tmp/adminware/venv/lib/python3.5/site-packages/plumbum/commands/base.py", line 240, in run return p.run() File "/tmp/adminware/venv/lib/python3.5/site-packages/plumbum/commands/base.py", line 201, in runner return run_proc(p, retcode, timeout) File "/tmp/adminware/venv/lib/python3.5/site-packages/plumbum/commands/processes.py", line 232, in run_proc return _check_process(proc, retcode, timeout, stdout, stderr) File "/tmp/adminware/venv/lib/python3.5/site-packages/plumbum/commands/processes.py", line 23, in _check_process proc.verify(retcode, timeout, stdout, stderr) File "/tmp/adminware/venv/lib/python3.5/site-packages/plumbum/machines/base.py", line 26, in verify stderr) plumbum.commands.processes.ProcessExecutionError: Command line: ['/usr/bin/nodeattr', '-f', '/tmp/tmp15x8hkam', '--expand'] Exit code: 1 Stderr: | nodeattr: /tmp/tmp15x8hkam: genders file parse error | nodeattr: use --parse-check to debug errors ```

@ColonelPanicks please can you confirm to what extent this is an issue & if we need to re-implement functionality to allow nodenames w/ periods Either way I will surround this in a try-except block shortly

[DavidMarchant]

It's worth noting that as a result of the same functionality you can now comma separate lists of nodes! e.g. --node node1,node2,node4 is now valid so :tada: :tada: i guess?

[ColonelPanics]

@DavidMarchant I have no problem with using --node with multiple nodenames and genders-friendly format. There shouldn't be need for periods in hostnames either so it's all good to me.

[DavidMarchant]

Cheers, great :)

[WilliamMcCumstie]

We can reasonably assume that a general user won't be trying to break the system. So a try-catch block should be sufficient for the odd case.

@mjtko That is unless genders could become the source of an injection attack? I don't believe so as it is mainly doing expansions.

[mjtko]

If we're passing unsanitized user input to external commands, we should certainly be testing for the possibility of an injection attack. Maybe it's a shlex quoting thing again, worth a bit of testing around this to make sure!

[WilliamMcCumstie]

In this case, we can take advantage of the fact that genders files have rather precise syntax. As commas and spaces have defined meanings, these shouldn't be allowed (in addition to anything dangerous). EDIT: Commas are probably fine, BUT no spaces

This limits the valid characters to the genders file to: /a-Z0-9,[]/ It might just be easier to limit the input to those characters (possible with click)? A try-catch might still be required by some garbage edge cases.

We don't want to give the user the ability to define groups during the genders expansion (even though this wouldn't be an issue in itself).