[refactor] move TunnelManager duties to VPNService

VPNService has knowledge of the tunnel and the firewall and controls
them separately. Also exceptions from VPNControl are handled locally
instead of being propagated.

- Resolves: #8976
- Related: #8942

3 files changed, 47 insertions, 145 deletions

diff --git a/src/leap/bitmask/vpn/_control.py b/src/leap/bitmask/vpn/_control.py
index a57c6e47..7f80f40b 100644
--- a/src/leap/bitmask/vpn/_control.py
+++ b/src/leap/bitmask/vpn/_control.py
@@ -66,17 +66,23 @@ class VPNControl(object):
             vpnproc.preUp()
         except Exception as e:
             self.log.error('Error on vpn pre-up {0!r}'.format(e))
-            raise
+            return False
         try:
             cmd = vpnproc.getCommand()
         except Exception as e:
             self.log.error(
                 'Error while getting vpn command... {0!r}'.format(e))
-            raise
+            return False
 
         env = os.environ
 
-        runningproc = reactor.spawnProcess(vpnproc, cmd[0], cmd, env)
+        try:
+            runningproc = reactor.spawnProcess(vpnproc, cmd[0], cmd, env)
+        except Exception as e:
+            self.log.error(
+                'Error while spwanning vpn process... {0!r}'.format(e))
+            return False
+
         vpnproc.pid = runningproc.pid
         self._vpnproc = vpnproc
 
diff --git a/src/leap/bitmask/vpn/service.py b/src/leap/bitmask/vpn/service.py
index 70c26248..075295cf 100644
--- a/src/leap/bitmask/vpn/service.py
+++ b/src/leap/bitmask/vpn/service.py
@@ -24,9 +24,12 @@ import os
 
 from time import strftime
 from twisted.internet import defer
+from twisted.logger import Logger
 
 from leap.bitmask.hooks import HookableService
-from leap.bitmask.vpn.tunnelmanager import TunnelManager
+from leap.bitmask.util import merge_status
+from leap.bitmask.vpn.fw.firewall import FirewallManager
+from leap.bitmask.vpn.tunnel import VPNTunnel
 from leap.bitmask.vpn._checks import is_service_ready, get_vpn_cert_path
 from leap.bitmask.vpn import privilege, helpers
 from leap.bitmask.vpn.privilege import NoPolkitAuthAgentAvailable
@@ -45,6 +48,7 @@ class VPNService(HookableService):
 
     name = 'vpn'
     _last_vpn_path = os.path.join('leap', 'last_vpn')
+    log = Logger()
 
     def __init__(self, basepath=None):
         """
@@ -52,8 +56,8 @@ class VPNService(HookableService):
         """
         super(VPNService, self).__init__()
 
-        self._started = False
-        self._tunnelmanager = None
+        self._tunnel = None
+        self._firewall = None
         self._domain = ''
 
         if basepath is None:
@@ -75,7 +79,7 @@ class VPNService(HookableService):
 
     @defer.inlineCallbacks
     def start_vpn(self, domain=None):
-        if self._started:
+        if self.do_status()['status'] == 'on':
             exc = Exception('VPN already started')
             exc.expected = True
             raise exc
@@ -89,7 +93,14 @@ class VPNService(HookableService):
         yield self._setup(domain)
 
         try:
-            started = self._tunnelmanager.start()
+            fw_ok = self._firewall.start()
+            if not fw_ok:
+                raise Exception('Could not start firewall')
+
+            vpn_ok = self._tunnel.start()
+            if not vpn_ok:
+                self._firewall.stop()
+                raise Exception('Could not start VPN')
 
         # XXX capture it inside start method
         # here I'd like to get (status, message)
@@ -98,37 +109,36 @@ class VPNService(HookableService):
             raise e
         # --------------------------------------
 
-        self._started = started
         self._domain = domain
         self._write_last(domain)
-        if started:
-            defer.returnValue({'result': 'started'})
-        else:
-            raise Exception('Could not start VPN, check logs')
+        defer.returnValue({'result': 'started'})
 
     def stop_vpn(self):
-        if not self._tunnelmanager:
-            raise Exception('VPN was not running')
+        if self._firewall and self._firewall.is_up():
+            fw_ok = self._firewall.stop()
+            if not fw_ok:
+                self.log.error("Firewall: error stopping")
 
-        if self._started:
-            self._tunnelmanager.stop()
-            self._started = False
-            return {'result': 'vpn stopped'}
-        elif self._tunnelmanager.is_firewall_up():
-            self._tunnelmanager.stop_firewall()
-            return {'result': 'firewall stopped'}
-        else:
+        if not self._tunnel or self._tunnel.status['status'] is not 'on':
             raise Exception('VPN was not running')
 
+        vpn_ok = self._tunnel.stop()
+        if not vpn_ok:
+            self.log.error("VPN: error stopping")
+
+        return {'result': 'vpn stopped'}
+
     def do_status(self):
-        status = {
-            'status': 'off',
-            'error': None,
-            'childrenStatus': {}
+        childrenStatus = {
+            'vpn': {'status': 'off', 'error': None},
+            'firewall': {'status': 'off', 'error': None},
         }
 
-        if self._tunnelmanager:
-            status = self._tunnelmanager.get_status()
+        if self._tunnel:
+            childrenStatus['vpn'] = self._tunnel.status
+        if self._firewall:
+            childrenStatus['firewall'] = self._firewall.status
+        status = merge_status(childrenStatus)
 
         if self._domain:
             status['domain'] = self._domain
@@ -202,8 +212,9 @@ class VPNService(HookableService):
                 'Cannot find provider certificate. '
                 'Please configure provider.')
 
-        self._tunnelmanager = TunnelManager(
+        self._tunnel = VPNTunnel(
             provider, remotes, cert_path, key_path, ca_path, extra_flags)
+        self._firewall = FirewallManager(remotes)
 
     def _cert_expires(self, provider):
         path = os.path.join(self._basepath, "leap", "providers", provider,
diff --git a/src/leap/bitmask/vpn/tunnelmanager.py b/src/leap/bitmask/vpn/tunnelmanager.py
deleted file mode 100644
index 4fb50047..00000000
--- a/src/leap/bitmask/vpn/tunnelmanager.py
+++ /dev/null
@@ -1,115 +0,0 @@
-#!/usr/bin/env python
-# -*- coding: utf-8 -*-
-# cli.py
-# Copyright (C) 2015 LEAP
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-from colorama import Fore
-
-from leap.bitmask.util import merge_status
-
-from leap.bitmask.vpn.fw.firewall import FirewallManager
-from leap.bitmask.vpn.tunnel import VPNTunnel
-
-
-# TODO further refactor pending: merge with VPNService?
-
-
-class TunnelManager(object):
-
-    """
-    A TunnelManager controls a VPNTunnel and the Firewall
-    """
-
-    def __init__(self, provider, remotes, cert, key, ca, flags):
-
-        self._vpntunnel = VPNTunnel(
-            provider, remotes, cert, key, ca, flags)
-        self._firewall = FirewallManager(remotes)
-        self.starting = False
-
-    def start(self):
-        # TODO we should have some way of switching this flag to False
-        # other than parsing the result of the status command.
-        self.starting = True
-        print(Fore.BLUE + "Firewall: starting..." + Fore.RESET)
-        fw_ok = self._firewall.start()
-        if not fw_ok:
-            print(Fore.RED + "Firewall: problem!")
-            self.starting = False
-            return False
-        print(Fore.GREEN + "Firewall: started" + Fore.RESET)
-
-        try:
-            vpn_ok = self._vpntunnel.start()
-        except Exception:
-            self.starting = False
-            return False
-
-        if not vpn_ok:
-            print (Fore.RED + "VPN: Error starting." + Fore.RESET)
-            self._firewall.stop()
-            print(Fore.GREEN + "Firewall: stopped." + Fore.RESET)
-            self.starting = False
-            return False
-        print(Fore.GREEN + "VPN: started" + Fore.RESET)
-        return True
-
-    def stop(self):
-        self.starting = False
-        print(Fore.BLUE + "Firewall: stopping..." + Fore.RESET)
-        fw_ok = self._firewall.stop()
-
-        if not fw_ok:
-            print (Fore.RED + "Firewall: Error stopping." + Fore.RESET)
-            return False
-
-        print(Fore.GREEN + "Firewall: stopped." + Fore.RESET)
-        print(Fore.BLUE + "VPN: stopping..." + Fore.RESET)
-
-        vpn_ok = self._vpntunnel.stop()
-        if not vpn_ok:
-            print (Fore.RED + "VPN: Error stopping." + Fore.RESET)
-            return False
-
-        print(Fore.GREEN + "VPN: stopped." + Fore.RESET)
-        return True
-
-    def stop_firewall(self):
-        self._firewall.stop()
-
-    def is_firewall_up(self):
-        return self._firewall.is_up()
-
-    def get_status(self):
-        childrenStatus = {
-            "vpn": self._vpntunnel.status,
-            "firewall": self._firewall.status
-        }
-        if self.starting:
-            # XXX small correction to the merge: if we are starting fw+vpn,
-            # we report vpn as starting so that is consistent with the ui or
-            # cli action. this state propagates from the parent
-            # object to the vpn child, and we revert it when we reach
-            # the 'on' state. this needs to be revisited in the formal state
-            # machine, and mainly needs a way of setting that state directly
-            # and resetting the 'starting' flag without resorting to hijack
-            # this command.
-            vpnstatus = childrenStatus['vpn']['status']
-            if vpnstatus == 'off':
-                childrenStatus['vpn']['status'] = 'starting'
-            if vpnstatus == 'on':
-                self.starting = False
-        return merge_status(childrenStatus)