Merge into trunk : ssh-agent : Code : juju-quickstart

Status:

Merged

Approved by:

Brad Crittenden on 2014-02-03

Approved revision:

54

Merged at revision:

53

Proposed branch:

lp://qastaging/~bac/juju-quickstart/ssh-agent

Merge into:

lp://qastaging/juju-quickstart

Diff against target:

220 lines (+64/-30)

3 files modified

Makefile (+4/-0)
quickstart/app.py (+7/-0)
quickstart/tests/test_app.py (+53/-30)

To merge this branch:

bzr merge lp://qastaging/~bac/juju-quickstart/ssh-agent

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Madison Scott-Clary (community)		2014-01-31	Approve on 2014-01-31
Review via email: mp+204348@code.qastaging.launchpad.net

Description of the change

Don't start a new ssh-agent unless needed.

If quickstart creates its own ssh-agent it renders a previously running one
unusable. Keys that were already added must be added again. And, when the
quikstart process dies, the environment variables it had set up as required by
ssh-agent are not available to the parent shell, so that agent effectively
becomes a zombie.

Instead, check for the existence of a running agent before spinning up a new
one. If none exists, then launch a new one and then check again to ensure it
is running.

https://codereview.appspot.com/53740045/

Revision history for this message

Brad Crittenden (bac) wrote on 2014-01-31:

#

Download full text (8.1 KiB)

Reviewers: mp+204348_code.launchpad.net,

Message:
Please take a look.

Description:
Don't start a new ssh-agent unless needed.

If quickstart creates its own ssh-agent it renders a previously running
one
unusable. Keys that were already added must be added again. And, when
the
quikstart process dies, the environment variables it had set up as
required by
ssh-agent are not available to the parent shell, so that agent
effectively
becomes a zombie.

Instead, check for the existence of a running agent before spinning up a
new
one. If none exists, then launch a new one and then check again to
ensure it
is running.

https://code.launchpad.net/~bac/juju-quickstart/ssh-agent/+merge/204348

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/53740045/

Affected files (+46, -15 lines):
   M Makefile
   A [revision details]
   M quickstart/app.py
   M quickstart/tests/test_app.py

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2014-01-31 18:09:48 +0000
+++ Makefile 2014-01-31 21:00:06 +0000
@@ -54,6 +54,7 @@
   @echo ' environment will be used. It is possible to override the default'
   @echo ' Juju environment by setting the JUJU_ENV environment variable,'
   @echo ' e.g.: "make run JUJU_ENV=ec2".'
+ @echo 'make debug - Same as the "run" target but with the --debug flag.\n'
   @echo 'make clean - Get rid of bytecode files, build and dist dirs, venv.'
   @echo 'make release - Register and upload a release on PyPI.'

@@ -68,6 +69,9 @@
$(PYTHON) setup.py register sdist upload

run: setup
+ $(VENV)/bin/python ./juju-quickstart
+
+debug: setup
$(VENV)/bin/python ./juju-quickstart --debug

source:

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: quickstart/app.py
=== modified file 'quickstart/app.py'
--- quickstart/app.py 2014-01-31 16:35:01 +0000
+++ quickstart/app.py 2014-01-31 21:00:06 +0000
@@ -96,11 +96,18 @@
      Allow the user to let quickstart create SSH keys, or quit by raising a
      ProgramExit if they would like to create the key themselves.
      """
+ # Test to see if we have ssh-keys loaded into the ssh-agent, or if we
can
+ # add them to the currently running ssh-agent.
+ if check_ssh_keys():
+ return
+
+ # No responsive agent was found. Start one up.
      try:
          utils.start_ssh_agent()
      except OSError as err:
          raise ProgramExit(bytes(err))

+ # And now check again.
      if not check_ssh_keys():
          print('Warning: No SSH keys were found in ~/.ssh\n\n'
                'To proceed and generate keys, quickstart can\n\n'

Index: quickstart/tests/test_app.py
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py 2014-01-31 16:35:01 +0000
+++ quickstart/tests/test_app.py 2014-01-31 21:00:06 +0000
@@ -192,23 +192,41 @@
'quickstart:\n\n ssh-keygen -b 4096 -t rsa'

def patch_check_ssh_keys(self,...

Reviewers: mp+204348_code.launchpad.net,

Message:
Please take a look.

Description:
Don't start a new ssh-agent unless needed.

If quickstart creates its own ssh-agent it renders a previously running
one
unusable.  Keys that were already added must be added again.  And, when
the
quikstart process dies, the environment variables it had set up as
required by
ssh-agent are not available to the parent shell, so that agent
effectively
becomes a zombie.

Instead, check for the existence of a running agent before spinning up a
new
one.  If none exists, then launch a new one and then check again to
ensure it
is running.

https://code.launchpad.net/~bac/juju-quickstart/ssh-agent/+merge/204348

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/53740045/

Affected files (+46, -15 lines):
   M Makefile
   A [revision details]
   M quickstart/app.py
   M quickstart/tests/test_app.py

Index: Makefile
=== modified file 'Makefile'
--- Makefile	2014-01-31 18:09:48 +0000
+++ Makefile	2014-01-31 21:00:06 +0000
@@ -54,6 +54,7 @@
  	@echo '  environment will be used. It is possible to override the default'
  	@echo '  Juju environment by setting the JUJU_ENV environment variable,'
  	@echo '  e.g.: "make run JUJU_ENV=ec2".'
+	@echo 'make debug - Same as the "run" target but with the --debug flag.\n'
  	@echo 'make clean - Get rid of bytecode files, build and dist dirs, venv.'
  	@echo 'make release - Register and upload a release on PyPI.'

@@ -68,6 +69,9 @@
  	$(PYTHON) setup.py register sdist upload

run: setup
+	$(VENV)/bin/python ./juju-quickstart
+
+debug: setup
  	$(VENV)/bin/python ./juju-quickstart --debug

source:

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: jeff.pihach@canonical.com-20140131205525-l4wcoiu9dpgt12q8
+New revision: bac@canonical.com-20140131210006-qngkfuu006rskk14

Index: quickstart/app.py
=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-01-31 16:35:01 +0000
+++ quickstart/app.py	2014-01-31 21:00:06 +0000
@@ -96,11 +96,18 @@
      Allow the user to let quickstart create SSH keys, or quit by raising a
      ProgramExit if they would like to create the key themselves.
      """
+    # Test to see if we have ssh-keys loaded into the ssh-agent, or if we  
can
+    # add them to the currently running ssh-agent.
+    if check_ssh_keys():
+        return
+
+    # No responsive agent was found.  Start one up.
      try:
          utils.start_ssh_agent()
      except OSError as err:
          raise ProgramExit(bytes(err))

+    # And now check again.
      if not check_ssh_keys():
          print('Warning: No SSH keys were found in ~/.ssh\n\n'
                'To proceed and generate keys, quickstart can\n\n'

Index: quickstart/tests/test_app.py
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-01-31 16:35:01 +0000
+++ quickstart/tests/test_app.py	2014-01-31 21:00:06 +0000
@@ -192,23 +192,41 @@
                 'quickstart:\n\n  ssh-keygen -b 4096 -t rsa'

def patch_check_ssh_keys(self, return_value=False):
-        mock_check_ssh_keys = mock.Mock(return_value=return_value)
-        return mock.patch('quickstart.app.check_ssh_keys',  
mock_check_ssh_keys)
+        mock_ = mock.Mock(return_value=return_value)
+        return mock.patch('quickstart.app.check_ssh_keys', mock_)

def patch_raw_input(self, return_value='C'):
-        mock_raw_input = mock.Mock(return_value=return_value)
-        return mock.patch('__builtin__.raw_input', mock_raw_input)
+        mock_ = mock.Mock(return_value=return_value)
+        return mock.patch('__builtin__.raw_input', mock_)
+
+    def patch_start_ssh_agent(self, return_value=False, side_effect=None):
+        mock_ = mock.Mock(return_value=return_value,  
side_effect=side_effect)
+        return mock.patch('quickstart.utils.start_ssh_agent', mock_)

def test_success(self, mock_print):
          with self.patch_check_ssh_keys(return_value=True) as mock_check:
              app.ensure_ssh_keys()
-        self.assertTrue(mock_check.called)
+        self.assertEqual(1, mock_check.call_count)

def test_error_starting_agent(self, mock_print):
          with self.assert_program_exit('foo'):
-            with mock.patch('quickstart.utils.start_ssh_agent',
-                            side_effect=OSError('foo')):
-                app.ensure_ssh_keys()
+            with self.patch_check_ssh_keys():
+                with  
self.patch_start_ssh_agent(side_effect=OSError('foo')):
+                    app.ensure_ssh_keys()
+
+    def test_extant_agent_returns(self, mock_print):
+        with self.patch_check_ssh_keys(True):
+            with self.patch_start_ssh_agent(
+                    side_effect=OSError('foo')) as mock_ssa:
+                app.ensure_ssh_keys()
+        self.assertFalse(mock_ssa.called)
+
+    def test_successful_agent_start(self, mock_print):
+        mock_check_ssh_keys = mock.Mock(side_effect=(False, True))
+        with mock.patch('quickstart.app.check_ssh_keys',  
mock_check_ssh_keys):
+            with self.patch_start_ssh_agent(return_value=True):
+                app.ensure_ssh_keys()
+        self.assertFalse(mock_print.called)

def test_failure_no_keygen(self, mock_print):
          with mock.patch('sys.exit') as mock_exit:
@@ -259,7 +277,7 @@
          helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase):

def test_keys_and_agent(self):
-        with self.patch_call(0) as mock_call:
+        with self.patch_call(retcode=0) as mock_call:
              have_keys = app.check_ssh_keys()
          mock_call.assert_called_once_with('/usr/bin/ssh-add', '-l')
          self.assertTrue(have_keys)
@@ -305,7 +323,7 @@
          ])

def test_no_agent(self):
-        with self.patch_call(2) as mock_call:
+        with self.patch_call(retcode=2) as mock_call:
              have_keys = app.check_ssh_keys()
          mock_call.assert_called_once_with('/usr/bin/ssh-add', '-l')
          self.assertFalse(have_keys)
@@ -348,7 +366,7 @@

def test_success(self, mock_print):
          key_file = os.path.join(os.path.expanduser('~'), '.ssh', 'id_rsa')
-        with self.patch_call(0) as mock_call:
+        with self.patch_call(retcode=0) as mock_call:
              app.create_ssh_keys()
          mock_call.assert_has_calls([
              mock.call('/usr/bin/ssh-keygen',
@@ -363,7 +381,7 @@

def test_failure(self, mock_print):
          with self.assert_program_exit('error generating ssh key!  Oh no!'):
-            with self.patch_call(1, error='Oh no!') as mock_call:
+            with self.patch_call(retcode=1, error='Oh no!') as mock_call:
                  app.create_ssh_keys()
          self.assertTrue(mock_call.called)
          with self.assert_program_exit('error adding key to agent!  Oh  
no!'):
@@ -474,7 +492,7 @@

def test_bootstrap_failure(self, mock_print):
          # A ProgramExit is raised if an error occurs while bootstrapping.
-        with self.patch_call(1, error='bad wolf') as mock_call:
+        with self.patch_call(retcode=1, error='bad wolf') as mock_call:
              with self.assert_program_exit('bad wolf'):
                  app.bootstrap(self.env_name)
          mock_call.assert_called_once_with(
@@ -597,7 +615,7 @@
      def test_success(self):
          # The API URL is correctly returned.
          api_addresses = json.dumps(['api.example.com:17070', 'not-today'])
-        with self.patch_call(0, output=api_addresses) as mock_call:
+        with self.patch_call(retcode=0, output=api_addresses) as mock_call:
              api_url = app.get_api_url(self.env_name)
          self.assertEqual('wss://api.example.com:17070', api_url)
          mock_call.assert_called_once_with(
@@ -606,7 +624,7 @@

def test_failure(self):
          # A ProgramExit is raised if an error occurs retrieving the API  
URL.
-        with self.patch_call(1, error='bad wolf') as mock_call:
+        with self.patch_call(retcode=1, error='bad wolf') as mock_call:
              with self.assert_program_exit('bad wolf'):
                  app.get_api_url(self.env_name)
          mock_call.assert_called_once_with(

lp://qastaging/~bac/juju-quickstart/ssh-agent updated on 2014-01-31

54. By Brad Crittenden on 2014-01-31: Avoid calling system ssh-agent in tests

Revision history for this message

Brad Crittenden (bac) wrote on 2014-01-31:

#

Please take a look.

https://codereview.appspot.com/53740045/

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2014-01-31:

#

This looks good, thanks for finding a better way around this!

review: Approve

juju-quickstart

Merge lp://qastaging/~bac/juju-quickstart/ssh-agent into lp://qastaging/juju-quickstart

Commit message

Description of the change

Preview Diff

Subscribers