Bug lives after disconnect (#12)

* Containers are ran with an 'init' system now

* Containers are now deleted if ContainerShell binary terminates unexpectedly

* incremented version

VERSION

@@ -1 +1 @@
-2019.08.09
+2019.08.15

container_shell/container_shell.py

@@ -2,18 +2,21 @@
 """Here we put everything together!"""
 import os
 import sys
+import atexit
+import signal
+import functools
 import subprocess
 from pwd import getpwnam
 from getpass import getuser
 
 import docker
+import requests
 import dockerpty
 
 from container_shell.lib.config import get_config
 from container_shell.lib import utils, dockage
 
-#pylint: disable=R0915
-#pylint: disable=R0912
+#pylint: disable=R0914
 def main():
     """Entry point logic"""
     user_info = getpwnam(getuser())
@@ -63,16 +66,61 @@ def main():
         logger.exception(doh)
         utils.printerr("Failed to create login environment")
         sys.exit(1)
+    else:
+        cleanup = functools.partial(kill_container,
+                                    container,
+                                    config['config']['term_signal'],
+                                    logger)
+        atexit.register(cleanup)
+        # When OpenSSH detects that a client has disconnected, it'll send
+        # SIGHUP to the process ran when that client connected.
+        # If we don't handle this signal, then users who click the little "x"
+        # on their SSH application (instead of pressing "CTL D" or typing "exit")
+        # will cause ContainerShell to leak containers. In other words, the
+        # SSH session will be gone, but the container will remain.
+        signal.signal(signal.SIGHUP, cleanup)
     try:
         dockerpty.start(docker_client.api, container.id)
     except Exception as doh: #pylint: disable=W0703
         logger.exception(doh)
         utils.printerr("Failed to connect to PTY")
         sys.exit(1)
+
+
+def kill_container(container, the_signal, logger):
+    """Tear down the container when ContainerShell exits
+
+    :Returns: None
+
+    :param container: The container created by ContainerShell
+    :type container: docker.models.containers.Container
+
+    :param the_signal: The Linux SIGNAL to set to the container in order to stop it.
+                       See "man signal" for details. Defaults to SIGHUP.
+    :type the_signal: String
+
+    :param logger: An object for writing errors/messages for debugging problems
+    :type logger: logging.Logger
+    """
+    try:
+        container.exec_run('kill -{} 1'.format(the_signal))
+    except requests.exceptions.HTTPError as doh:
+        if doh.response.status_code == 409:
+            # the container is already stopped
+            pass
+        else:
+            logger.exception(doh)
     try:
         container.kill()
-    except docker.errors.NotFound:
-        pass
+    except requests.exceptions.HTTPError as doh:
+        status_code = doh.response.status_code
+        #pylint: disable=R1714
+        if status_code == 404 or status_code == 409:
+            # Container is already deleted, or stopped
+            pass
+        else:
+            logger.info(dir(doh))
+            logger.exception(doh)
     except Exception as doh: #pylint: disable=W0703
         logger.exception(doh)
     try:

container_shell/lib/config.py

@@ -47,6 +47,7 @@ def _default():
     config.set('config', 'create_user', 'true')
     config.set('config', 'disable_scp', '')
     config.set('config', 'command', '')
+    config.set('config', 'term_signal', 'SIGHUP')
     config.set('logging', 'location', '/var/log/container_shell/messages.log')
     config.set('logging', 'max_size', '1024000') # 1MB
     config.set('logging', 'max_count', '3')

container_shell/lib/dockage.py

@@ -18,6 +18,7 @@ def build_args(config, username, user_uid, logger):
         'image' : config['config'].get('image'),
         'hostname' : config['config'].get('hostname'),
         'tty' : True,
+        'init' : True,
         'stdin_open' : True,
         'dns' : dns(config['dns']['servers']),
         'mounts' : mounts(config['mounts']),

sample.config.ini

@@ -23,6 +23,11 @@ disable_scp=true
 # use here, make sure to supply an absolute path
 #command=/bin/bash
 
+# Most shells terminate upon receiving SIGHUP. If you shell doesn't, change this
+# to the approprete signal. If you don't, you might "leak containers" when a
+# user's session suddenly disappears.
+term_signal=SIGHUP
+
 # Adjust the logging parameters here. Omit a section to use the default value.
 [logging]
 location=/var/log/container_shell/messages.log

tests/test_config.py

@@ -27,6 +27,7 @@ def test_defaults(self):
         test_config.set('config', 'create_user', 'true')
         test_config.set('config', 'disable_scp', '')
         test_config.set('config', 'command', '')
+        test_config.set('config', 'term_signal', 'SIGHUP')
         test_config.set('logging', 'location', '/var/log/container_shell/messages.log')
         test_config.set('logging', 'max_size', '1024000') # 1MB
         test_config.set('logging', 'max_count', '3')

tests/test_container_shell.py

@@ -4,6 +4,7 @@
 from unittest.mock import patch, MagicMock
 
 import docker
+import requests
 
 from container_shell import container_shell
 from container_shell.lib.config import _default
@@ -231,92 +232,129 @@ def test_pty_failure(self, fake_printerr, fake_dockage, fake_docker_from_env,
 
         self.assertEqual(error_msg, expected_msg)
 
-    @patch.object(container_shell.utils, 'get_logger')
-    @patch.object(container_shell, 'get_config')
-    @patch.object(container_shell, 'dockerpty')
-    @patch.object(container_shell.docker, 'from_env')
-    @patch.object(container_shell, 'dockage')
-    @patch.object(container_shell.utils, 'printerr')
-    def test_kill_failure(self, fake_printerr, fake_dockage, fake_docker_from_env,
-                         fake_dockerpty, fake_get_config, fake_get_logger):
-        """``container_shell`` Ignores failures to kill a container because it's already been killed"""
-        fake_get_config.return_value = (_default(), True, '')
+    def test_kill_container(self):
+        """``container_shell`` 'kill_container' only logs if there's an unexpected error"""
         fake_container = MagicMock()
-        fake_container.kill.side_effect = docker.errors.NotFound('Testing')
-        fake_docker_client = MagicMock()
-        fake_docker_client.containers.create.return_value = fake_container
-        fake_docker_from_env.return_value = fake_docker_client
-
-        container_shell.main()
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
-        self.assertTrue(fake_container.kill.called)
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        # If you call hasattr() on MagicMock, it'll add the addr, but dir() doesn't
+        attrs = dir(fake_logger)
+        self.assertFalse(fake_logger.called)
+        self.assertFalse('exception' in attrs)
+        self.assertFalse('error' in attrs)
+        self.assertFalse('info' in attrs)
 
-    @patch.object(container_shell.utils, 'get_logger')
-    @patch.object(container_shell, 'get_config')
-    @patch.object(container_shell, 'dockerpty')
-    @patch.object(container_shell.docker, 'from_env')
-    @patch.object(container_shell, 'dockage')
-    @patch.object(container_shell.utils, 'printerr')
-    def test_kill_unknown_failure(self, fake_printerr, fake_dockage, fake_docker_from_env,
-                                  fake_dockerpty, fake_get_config, fake_get_logger):
-        """``container_shell``  Logs unexpected errors when terminating/killing a container"""
-        fake_get_config.return_value = (_default(), True, '')
-        fake_logger = MagicMock()
-        fake_get_logger.return_value = fake_logger
+    def test_kill_container_exec_run_error_ignore(self):
+        """``container_shell`` 'kill_container' ignore errors if the container is already stopped"""
         fake_container = MagicMock()
-        fake_container.kill.side_effect = Exception('Testing')
-        fake_docker_client = MagicMock()
-        fake_docker_client.containers.create.return_value = fake_container
-        fake_docker_from_env.return_value = fake_docker_client
+        fake_resp = MagicMock()
+        fake_resp.status_code = 409
+        fake_container.exec_run.side_effect = [requests.exceptions.HTTPError('testing', response=fake_resp)]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
-        container_shell.main()
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        # If you call hasattr() on MagicMock, it'll add the addr, but dir() doesn't
+        attrs = dir(fake_logger)
+        self.assertFalse(fake_logger.called)
+        self.assertFalse('exception' in attrs)
+        self.assertFalse('error' in attrs)
+        self.assertFalse('info' in attrs)
 
+    def test_kill_container_exec_run_error(self):
+        """``container_shell`` 'kill_container' logs unexpected errors when executing 'kill' in the container"""
+        fake_container = MagicMock()
+        fake_resp = MagicMock()
+        fake_resp.status_code = 500
+        fake_container.exec_run.side_effect = [requests.exceptions.HTTPError('testing', response=fake_resp)]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
+
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
         self.assertTrue(fake_logger.exception.called)
 
+    def test_kill_container_kill_ignore_404(self):
+        """``container_shell`` 'kill_container' ignore expected errors when killing the container"""
+        fake_container = MagicMock()
+        fake_resp = MagicMock()
+        fake_resp.status_code = 404
+        fake_container.kill.side_effect = [requests.exceptions.HTTPError('testing', response=fake_resp)]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        # If you call hasattr() on MagicMock, it'll add the addr, but dir() doesn't
+        attrs = dir(fake_logger)
+        self.assertFalse(fake_logger.called)
+        self.assertFalse('exception' in attrs)
+        self.assertFalse('error' in attrs)
+        self.assertFalse('info' in attrs)
 
-    @patch.object(container_shell.utils, 'get_logger')
-    @patch.object(container_shell, 'get_config')
-    @patch.object(container_shell, 'dockerpty')
-    @patch.object(container_shell.docker, 'from_env')
-    @patch.object(container_shell, 'dockage')
-    @patch.object(container_shell.utils, 'printerr')
-    def test_remove_failure(self, fake_printerr, fake_dockage, fake_docker_from_env,
-                         fake_dockerpty, fake_get_config, fake_get_logger):
-        """``container_shell`` Ignores failures to delete a container because it's already been deleted"""
-        fake_get_config.return_value = (_default(), True, '')
+    def test_kill_container_kill_ignore_409(self):
+        """``container_shell`` 'kill_container' ignore expected errors when killing the container"""
         fake_container = MagicMock()
-        fake_container.remove.side_effect = docker.errors.NotFound('Testing')
-        fake_docker_client = MagicMock()
-        fake_docker_client.containers.create.return_value = fake_container
-        fake_docker_from_env.return_value = fake_docker_client
+        fake_resp = MagicMock()
+        fake_resp.status_code = 409
+        fake_container.kill.side_effect = [requests.exceptions.HTTPError('testing', response=fake_resp)]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
-        container_shell.main()
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        # If you call hasattr() on MagicMock, it'll add the addr, but dir() doesn't
+        attrs = dir(fake_logger)
+        self.assertFalse(fake_logger.called)
+        self.assertFalse('exception' in attrs)
+        self.assertFalse('error' in attrs)
+        self.assertFalse('info' in attrs)
 
-        self.assertTrue(fake_container.kill.called)
+    def test_kill_container_kill_logs(self):
+        """``container_shell`` 'kill_container' logs unexpected errors when killing the container"""
+        fake_container = MagicMock()
+        fake_resp = MagicMock()
+        fake_resp.status_code = 500
+        fake_container.kill.side_effect = [requests.exceptions.HTTPError('testing', response=fake_resp)]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
-    @patch.object(container_shell.utils, 'get_logger')
-    @patch.object(container_shell, 'get_config')
-    @patch.object(container_shell, 'dockerpty')
-    @patch.object(container_shell.docker, 'from_env')
-    @patch.object(container_shell, 'dockage')
-    @patch.object(container_shell.utils, 'printerr')
-    def test_remove_unknown_failure(self, fake_printerr, fake_dockage, fake_docker_from_env,
-                                    fake_dockerpty, fake_get_config, fake_get_logger):
-        """``container_shell`` Logs unexpected errors when deleting a container"""
-        fake_get_config.return_value = (_default(), True, '')
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        self.assertTrue(fake_logger.exception.called)
+
+    def test_kill_container_kill_exception(self):
+        """``container_shell`` 'kill_container' logs generic exception errors when killing the container"""
+        fake_container = MagicMock()
+        fake_container.kill.side_effect = [RuntimeError('testing')]
+        the_signal = 'SIGHUP'
         fake_logger = MagicMock()
-        fake_get_logger.return_value = fake_logger
+
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        self.assertTrue(fake_logger.exception.called)
+
+    def test_remove_not_found(self):
+        """``container_shell`` 'kill_container' ignores errors when trying to delete an already deleted container"""
         fake_container = MagicMock()
-        fake_container.remove.side_effect = Exception('Testing')
-        fake_docker_client = MagicMock()
-        fake_docker_client.containers.create.return_value = fake_container
-        fake_docker_from_env.return_value = fake_docker_client
+        fake_container.remove.side_effect = [docker.errors.NotFound('testing')]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
-        container_shell.main()
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        # If you call hasattr() on MagicMock, it'll add the addr, but dir() doesn't
+        attrs = dir(fake_logger)
+        self.assertFalse(fake_logger.called)
+        self.assertFalse('exception' in attrs)
+        self.assertFalse('error' in attrs)
+        self.assertFalse('info' in attrs)
 
-        self.assertTrue(fake_logger.exception.called)
+    def test_remove_exception(self):
+        """``container_shell`` 'kill_container' logs unexpected errors when trying to delete a container"""
+        fake_container = MagicMock()
+        fake_container.remove.side_effect = [RuntimeError('testing')]
+        the_signal = 'SIGHUP'
+        fake_logger = MagicMock()
 
+        container_shell.kill_container(fake_container, the_signal, fake_logger)
+        self.assertTrue(fake_logger.exception.called)
 
 
 if __name__ == '__main__':