Merge lp:~jimbaker/pyjuju/ssh-passthrough into lp:pyjuju

Proposed by Jim Baker
Status: Merged
Approved by: Kapil Thangavelu
Approved revision: 412
Merged at revision: 420
Proposed branch: lp:~jimbaker/pyjuju/ssh-passthrough
Merge into: lp:pyjuju
Diff against target: 393 lines (+219/-17)
6 files modified
juju/control/__init__.py (+20/-4)
juju/control/command.py (+2/-1)
juju/control/ssh.py (+32/-9)
juju/control/tests/test_ssh.py (+64/-2)
juju/control/tests/test_utils.py (+48/-1)
juju/control/utils.py (+53/-0)
To merge this branch: bzr merge lp:~jimbaker/pyjuju/ssh-passthrough
Reviewer Review Type Date Requested Status
Kapil Thangavelu (community) Approve
William Reade (community) Approve
Review via email: mp+79888@code.launchpad.net

Description of the change

Implements the ability to passthrough args to an underlying command, in particular to support juju ssh. This support enables using juju ssh in the following ways:

$ juju ssh mysql/0 ls /

$ juju ssh -L8080:localhost:80 wordpress/0

and so forth, any options are simply passed through, along with the ssh command.

This requires working around argparse a bit, but only through its public API. In particular, main now uses parse_known_args to see if the command module in question supports the passthrough function; it if it does, that function is used to further decorate options from any extra args. Then command execution proceeds as before.

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :

[0] trivial

+ "[passthru ssh options] unit_or_machine [command]"),

s/passthru/passthrough

[1] trivial

+ # workding with ssh, for example), so consume the rest and

s/workding/working

[2] less trivial

I didn't see any way to handle potential conflicts in args (specifically -o ControlMaster/ControlPath, which I think are the only ones we need to worry about at the moment). Not really a big deal, but something it might be worth logging a "low" bug against, to make it clear that the limitation is known but not a priority?

anyway, +1

review: Approve
Revision history for this message
Jim Baker (jimbaker) wrote :

> [0] trivial
>
> + "[passthru ssh options] unit_or_machine [command]"),
>
> s/passthru/passthrough

Thanks, let's keep it consistent. (Both seem to be used, but passthrough more so. Or perhaps pass-through.)

>
> [1] trivial
>
> + # workding with ssh, for example), so consume the rest and
>
> s/workding/working

Ack

>
> [2] less trivial
>
> I didn't see any way to handle potential conflicts in args (specifically -o
> ControlMaster/ControlPath, which I think are the only ones we need to worry
> about at the moment). Not really a big deal, but something it might be worth
> logging a "low" bug against, to make it clear that the limitation is known but
> not a priority?

Indeed, it's possible for the user to escape the walled garden and use ssh flags in bad ways as well as good. Passthrough flags are presented later to ssh, and can thereby override juju's default usage. In the particular example you cite, this enables the user to know what is best for a given scenario. For example, multiplexing a connection would not be so ideal if there's significant data transfer. Until now this was a moot point, but with tunnel support, this need could actually arise, and it can be simply solved by passing in -o "ControlMaster no".

I don't think it's a low bug, just simply a matter of adding to the documentation of this command.

>
> anyway, +1

Thanks!

Revision history for this message
Jim Baker (jimbaker) wrote :

> I don't think it's a low bug, just simply a matter of adding to the
> documentation of this command.

To be precise: it should be fixed in the context of this branch.

Revision history for this message
Jim Baker (jimbaker) wrote :

>
> > I don't think it's a low bug, just simply a matter of adding to the
> > documentation of this command.
>
> To be precise: it should be fixed in the context of this branch.

irc discussion:

<jimbaker> fwereade, i think i'm going to change my mind on the doc stuff related to juju ssh. the command itself is not documented. really we need to incorporate more detailed docs on each subcommand in juju
<jimbaker> fwereade, so that should be done in a separate branch/bug
<fwereade> jimbaker, sounds sensible to me

lp:~jimbaker/pyjuju/ssh-passthrough updated
410. By Jim Baker

Fix doc typo

411. By Jim Baker

Merged trunk

412. By Jim Baker

Addressed review points

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Nice work jim. lgtm, a minor

[0]

+def parse_passthrough_args(args, flags_taking_arg=frozenset()):

the frozenset seems like overkill when an empty tuple suffices.

[1]

re the same function.

Is supporting long options just a matter of an additional conditional
clause for peeked.startswith("--") ? If so it seems better to just
implement support for it now.

[2] re the conflict william brought up, experimenting with the ssh cli it doesn't actually appear to be a problem in practice the user specified option will take precedence as its specified last.

review: Approve
Revision history for this message
Jim Baker (jimbaker) wrote :

> Nice work jim. lgtm, a minor
>
> [0]
>
> +def parse_passthrough_args(args, flags_taking_arg=frozenset()):
>
> the frozenset seems like overkill when an empty tuple suffices.

Sure, definite overkill.

>
> [1]
>
> re the same function.
>
> Is supporting long options just a matter of an additional conditional
> clause for peeked.startswith("--") ? If so it seems better to just
> implement support for it now.

It's not quite so simple, and so far the universe of commands that need this passthrough support is as far as I know is just ssh/scp. So I'd suggest punting that support until we identify them.
>
>
> [2] re the conflict william brought up, experimenting with the ssh cli it
> doesn't actually appear to be a problem in practice the user specified option
> will take precedence as its specified last.

My experience too.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'juju/control/__init__.py'
2--- juju/control/__init__.py 2011-10-10 20:20:20 +0000
3+++ juju/control/__init__.py 2011-10-20 17:26:23 +0000
4@@ -4,6 +4,7 @@
5 import zookeeper
6
7 from .command import Commander
8+from .utils import ParseError
9 from juju.environment.config import EnvironmentsConfig
10
11 import add_relation
12@@ -106,13 +107,15 @@
13
14 for module in subcommands:
15 configure_subparser = getattr(module, "configure_subparser", None)
16+ passthrough = getattr(module, "passthrough", None)
17 if configure_subparser:
18 sub_parser = configure_subparser(subparsers)
19 else:
20 sub_parser = subparsers.add_parser(
21 module.__name__.split('.')[-1], help=module.command.__doc__)
22- sub_parser.set_defaults(command=Commander(module.command),
23- parser=sub_parser)
24+ sub_parser.set_defaults(
25+ command=Commander(module.command, passthrough=passthrough),
26+ parser=sub_parser)
27
28 return parser
29
30@@ -157,7 +160,20 @@
31 prog="juju",
32 description="juju cloud orchestration admin")
33 parser.set_defaults(environments=env_config, log=log)
34- options = parser.parse_args(args)
35+
36+ # Some commands, like juju ssh, do a further parse on options by
37+ # delegating to another command (such as the underlying ssh). But
38+ # first need to parse nonstrictly all args to even determine what
39+ # command is even being used.
40+ options, extra = parser.parse_known_args(args)
41+ if options.command.passthrough:
42+ try:
43+ # Augments options with subparser specific passthrough parsing
44+ options.command.passthrough(options, extra)
45+ except ParseError, e:
46+ options.parser.error(str(e))
47+ else:
48+ # Otherwise, do be strict
49+ options = parser.parse_args(args)
50 setup_logging(options)
51-
52 options.command(options)
53
54=== modified file 'juju/control/command.py'
55--- juju/control/command.py 2011-09-15 18:50:23 +0000
56+++ juju/control/command.py 2011-10-20 17:26:23 +0000
57@@ -19,12 +19,13 @@
58
59 """
60
61- def __init__(self, callback):
62+ def __init__(self, callback, passthrough=False):
63 if not callable(callback):
64 raise ValueError(
65 "Commander callback argument must be a callable")
66
67 self.callback = callback
68+ self.passthrough = passthrough
69 self.options = None
70 self.exit_code = 0
71
72
73=== modified file 'juju/control/ssh.py'
74--- juju/control/ssh.py 2011-09-25 14:05:19 +0000
75+++ juju/control/ssh.py 2011-10-20 17:26:23 +0000
76@@ -1,8 +1,10 @@
77+from argparse import RawDescriptionHelpFormatter
78 import os
79
80 from twisted.internet.defer import inlineCallbacks, returnValue
81
82-from juju.control.utils import get_environment
83+from juju.control.utils import (
84+ get_environment, parse_passthrough_args, ParseError)
85 from juju.state.errors import (
86 ServiceUnitStateMachineNotAssigned, MachineStateNotFound)
87 from juju.state.machine import MachineStateManager
88@@ -11,14 +13,34 @@
89
90
91 def configure_subparser(subparsers):
92- sub_parser = subparsers.add_parser("ssh", help=command.__doc__)
93- sub_parser.add_argument(
94- "unit_or_machine", help="Name of unit or machine")
95+ sub_parser = subparsers.add_parser(
96+ "ssh",
97+ help=command.__doc__,
98+ usage=("%(prog)s [-h] [--environment ENVIRONMENT] "
99+ "[passthrough ssh options] unit_or_machine [command]"),
100+ formatter_class=RawDescriptionHelpFormatter,
101+ description=(
102+ "positional arguments:\n"
103+ " unit_or_machine Name of unit or machine\n"
104+ " [command] Optional command to run on machine"))
105 sub_parser.add_argument(
106 "--environment", "-e", help="Environment to operate on.")
107 return sub_parser
108
109
110+def passthrough(options, extra):
111+ """Second parsing phase to parse `extra` to passthrough to ssh itself.
112+
113+ Partitions into flags, unit_or_machine, and optional ssh command.
114+ """
115+ flags, positional = parse_passthrough_args(extra, "bcDeFIiLlmOopRSWw")
116+ if not positional:
117+ raise ParseError("too few arguments")
118+ options.ssh_flags = flags
119+ options.unit_or_machine = positional.pop(0)
120+ options.ssh_command = positional # if any
121+
122+
123 @inlineCallbacks
124 def get_ip_address_for_unit(client, provider, unit_name):
125 manager = ServiceStateManager(client)
126@@ -41,13 +63,15 @@
127 returnValue((provider_machine.dns_name, machine_state))
128
129
130-def open_ssh(ip_address):
131+def open_ssh(flags, ip_address, ssh_command):
132 # XXX - TODO - Might be nice if we had the ability to get the user's
133 # private key path and utilize it here, ie the symmetric end to
134 # get user public key.
135 args = ["ssh"]
136 args.extend(prepare_ssh_sharing())
137+ args.extend(flags)
138 args.extend(["ubuntu@%s" % ip_address])
139+ args.extend(ssh_command)
140 os.execvp("ssh", args)
141
142
143@@ -58,10 +82,9 @@
144 environment = get_environment(options)
145 provider = environment.get_machine_provider()
146 client = yield provider.connect()
147-
148 label = machine = unit = None
149
150- # First check if its a juju machine id
151+ # First check if it's a juju machine id
152 if options.unit_or_machine.isdigit():
153 options.log.debug(
154 "Fetching machine address using juju machine id.")
155@@ -69,7 +92,7 @@
156 client, provider, options.unit_or_machine)
157 machine.get_ip_address = get_ip_address_for_machine
158 label = "machine"
159- # Next check if its a unit
160+ # Next check if it's a unit
161 elif "/" in options.unit_or_machine:
162 options.log.debug(
163 "Fetching machine address using unit name.")
164@@ -100,4 +123,4 @@
165
166 options.log.info("Connecting to %s %s at %s",
167 label, options.unit_or_machine, ip_address)
168- open_ssh(ip_address)
169+ open_ssh(options.ssh_flags, ip_address, options.ssh_command)
170
171=== modified file 'juju/control/tests/test_ssh.py'
172--- juju/control/tests/test_ssh.py 2011-09-25 14:05:19 +0000
173+++ juju/control/tests/test_ssh.py 2011-10-20 17:26:23 +0000
174@@ -58,7 +58,7 @@
175 @inlineCallbacks
176 def test_shell_with_unit(self):
177 """
178- 'juju shell mysql/0' will execute ssh against the machine
179+ 'juju ssh mysql/0' will execute ssh against the machine
180 hosting the unit.
181 """
182 mock_environment = self.mocker.patch(Environment)
183@@ -92,6 +92,46 @@
184 self.output.getvalue())
185
186 @inlineCallbacks
187+ def test_passthrough_args(self):
188+ """Verify that args are passed through to the underlying ssh command.
189+
190+ For example, something like the following command should be valid::
191+
192+ $ juju ssh -L8080:localhost:8080 -o "ConnectTimeout 60" mysql/0 ls /
193+ """
194+ mock_environment = self.mocker.patch(Environment)
195+ mock_environment.get_machine_provider()
196+ self.mocker.result(self.provider)
197+
198+ mock_exec = self.mocker.replace(os.execvp)
199+ mock_exec("ssh", [
200+ "ssh",
201+ "-o",
202+ "ControlPath " + self.tmp_home + "/.juju/ssh/master-%r@%h:%p",
203+ "-o", "ControlMaster no",
204+ "-L8080:localhost:8080", "-o", "ConnectTimeout 60",
205+ "ubuntu@mysql-0.example.com", "ls *"])
206+
207+ # Track unwanted calls:
208+ calls = []
209+ mock_exec(ARGS, KWARGS)
210+ self.mocker.count(0, None)
211+ self.mocker.call(lambda *args, **kwargs: calls.append((args, kwargs)))
212+
213+ finished = self.setup_cli_reactor()
214+ self.mocker.replay()
215+ yield self.unit.connect_agent()
216+
217+ main(["ssh", "-L8080:localhost:8080", "-o", "ConnectTimeout 60",
218+ self.unit.unit_name, "ls *"])
219+ yield finished
220+
221+ self.assertEquals(calls, [])
222+ self.assertIn(
223+ "Connecting to unit mysql/0 at mysql-0.example.com",
224+ self.output.getvalue())
225+
226+ @inlineCallbacks
227 def test_shell_with_unit_and_unconnected_unit_agent(self):
228 """If a unit doesn't have a connected unit agent,
229 the ssh command will wait till one exists before connecting.
230@@ -269,7 +309,7 @@
231 @inlineCallbacks
232 def test_shell_with_machine_id(self):
233 """
234- 'juju shell <machine_id>' will execute ssh against the machine
235+ 'juju ssh <machine_id>' will execute ssh against the machine
236 with the corresponding id.
237 """
238 mock_environment = self.mocker.patch(Environment)
239@@ -338,3 +378,25 @@
240 yield finished
241
242 self.assertIn("Machine 1 was not found", self.stderr.getvalue())
243+
244+
245+class ParseErrorsTest(ServiceStateManagerTestBase, ControlToolTest):
246+
247+ @inlineCallbacks
248+ def setUp(self):
249+ yield super(ParseErrorsTest, self).setUp()
250+ config = {
251+ "environments": {"firstenv": {"type": "dummy"}}}
252+
253+ self.write_config(dump(config))
254+ self.config.load()
255+ self.stderr = self.capture_stream("stderr")
256+
257+ def test_passthrough_args_parse_error(self):
258+ """Verify that bad passthrough args will get an argparse error."""
259+ e = self.assertRaises(
260+ SystemExit, main, ["ssh", "-L", "mysql/0"])
261+ self.assertEqual(e.code, 2)
262+ self.assertIn("juju ssh: error: too few arguments",
263+ self.stderr.getvalue())
264+
265
266=== modified file 'juju/control/tests/test_utils.py'
267--- juju/control/tests/test_utils.py 2011-09-15 19:24:47 +0000
268+++ juju/control/tests/test_utils.py 2011-10-20 17:26:23 +0000
269@@ -2,7 +2,8 @@
270
271 from juju.environment.errors import EnvironmentsConfigError
272 from juju.environment.config import EnvironmentsConfig
273-from juju.control.utils import get_environment
274+from juju.control.utils import (
275+ get_environment, parse_passthrough_args, ParseError)
276 from juju.control.tests.common import ControlToolTest
277
278
279@@ -65,3 +66,49 @@
280 options)
281
282 self.assertIn("Invalid environment 'volcano'", str(error))
283+
284+
285+class ParsePassthroughArgsTest(ControlToolTest):
286+
287+ def test_parse_typical_ssh(self):
288+ """Verify that flags and positional args are properly partitioned."""
289+ ssh_flags = "bcDeFIiLlmOopRSWw"
290+ self.assertEqual(
291+ parse_passthrough_args(
292+ ["-L8080:localhost:80", "-o", "Volume 11", "mysql/0", "ls a*"],
293+ ssh_flags),
294+ (["-L8080:localhost:80", "-o", "Volume 11"], ["mysql/0", "ls a*"]))
295+
296+ self.assertEqual(
297+ parse_passthrough_args(
298+ ["-L8080:localhost:80", "-aC26", "0", "foobar", "do", "123"],
299+ ssh_flags),
300+ (["-L8080:localhost:80", "-aC26"], ["0", "foobar", "do", "123"]))
301+
302+ self.assertEqual(
303+ parse_passthrough_args(["mysql/0"], ssh_flags),
304+ ([], ["mysql/0"]))
305+
306+ self.assertEqual(
307+ parse_passthrough_args(
308+ ["mysql/0", "command", "-L8080:localhost:80"], ssh_flags),
309+ ([], ["mysql/0", "command", "-L8080:localhost:80"]))
310+
311+ def test_parse_flag_taking_args(self):
312+ """Verify that arg-taking flags properly combine with args"""
313+ # some sample flags, from the ssh command
314+ ssh_flags = "bcDeFIiLlmOopRSWw"
315+
316+ for flag in ssh_flags:
317+ # This flag properly combines, either of the form -Xabc or -X abc
318+ self.assertEqual(
319+ parse_passthrough_args(
320+ ["-" + flag + "XYZ", "-1X", "-" + flag, "XYZ", "mysql/0"],
321+ ssh_flags),
322+ (["-" + flag + "XYZ", "-1X", "-" + flag, "XYZ"], ["mysql/0"]))
323+
324+ # And requires that it is combined
325+ e = self.assertRaises(
326+ ParseError,
327+ parse_passthrough_args, ["-" + flag], ssh_flags)
328+ self.assertEqual(str(e), "argument -%s: expected one argument" % flag)
329
330=== modified file 'juju/control/utils.py'
331--- juju/control/utils.py 2011-09-15 18:50:23 +0000
332+++ juju/control/utils.py 2011-10-20 17:26:23 +0000
333@@ -1,3 +1,5 @@
334+from itertools import tee
335+
336 from juju.environment.errors import EnvironmentsConfigError
337
338
339@@ -9,3 +11,54 @@
340 elif environment is None:
341 environment = options.environments.get_default()
342 return environment
343+
344+
345+class ParseError(Exception):
346+ """Used to support returning custom parse errors in passthrough parsing.
347+
348+ Enables similar support to what is seen in argparse, without using its
349+ internals.
350+ """
351+
352+
353+def parse_passthrough_args(args, flags_taking_arg=frozenset()):
354+ """Scans left to right, partitioning flags and positional args.
355+
356+ :param args: Unparsed args from argparse
357+ :param flags_taking_arg: One character flags that combine
358+ with arguments.
359+ :return: tuple of flags and positional args
360+ :raises: :class:`juju.control.utils.ParseError`
361+
362+ TODO May need to support long options for other passthrough commands.
363+ """
364+ args = iter(args)
365+ flags_taking_arg = set(flags_taking_arg)
366+ flags = []
367+ positional = []
368+ while True:
369+ args, peek_args = tee(args)
370+ try:
371+ peeked = peek_args.next()
372+ except StopIteration:
373+ break
374+ if peeked.startswith("-"):
375+ flags.append(args.next())
376+ # Only need to consume the next arg if the flag both takes
377+ # an arg (say -L) and then it has an extra arg following
378+ # (8080:localhost:80), rather than being combined, such as
379+ # -L8080:localhost:80
380+ if len(peeked) == 2 and peeked[1] in flags_taking_arg:
381+ try:
382+ flags.append(args.next())
383+ except StopIteration:
384+ raise ParseError(
385+ "argument -%s: expected one argument" % peeked[1])
386+ else:
387+ # At this point no more flags for the command itself (more
388+ # can follow after the first positional arg, as seen in
389+ # working with ssh, for example), so consume the rest and
390+ # stop parsing options
391+ positional = list(args)
392+ break
393+ return flags, positional

Subscribers

People subscribed via source and target branches

to status/vote changes: